You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2023/01/10 07:26:32 UTC
[GitHub] [maven] gnodet opened a new pull request, #950: Use proxies for session scoped beans
gnodet opened a new pull request, #950:
URL: https://github.com/apache/maven/pull/950
Following this checklist to help us incorporate your
contribution quickly and easily:
- [ ] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MNG) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [ ] Each commit in the pull request should have a meaningful subject line and body.
- [ ] Format the pull request title like `[MNG-XXX] SUMMARY`,
where you replace `MNG-XXX` and `SUMMARY` with the appropriate JIRA issue.
- [ ] Also format the first line of the commit message like `[MNG-XXX] SUMMARY`.
Best practice is to use the JIRA issue title in both the pull request title and in the first line of the commit message.
- [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [ ] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [ ] You have run the [Core IT][core-its] successfully.
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [ ] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
[core-its]: https://maven.apache.org/core-its/core-it-suite/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7662] Use proxies for session scoped beans [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #950:
URL: https://github.com/apache/maven/pull/950#issuecomment-1803785911
> Why is `@Typed` required? It has a serious side effect in Sisu regarding how bean is bound...
Oh, yes. That's actually the semantics that we need. The goal is to make sure the proxied bean is exposed only with interfaces. So we need `@Typed` in order to make sure it will be injected properly.
I experimented with adding an interface list to `@SessionScoped`, but then, if the injection point was using the actual implementation class instead of an interface, the injection would fail without a proper explanation (just that setting the field did not work, but not with a ClassCastException or anything that could give a hint).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7662] Use proxies for session scoped beans [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #950:
URL: https://github.com/apache/maven/pull/950#discussion_r1386757692
##########
maven-core/pom.xml:
##########
@@ -144,6 +144,12 @@ under the License.
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
+ <dependency>
Review Comment:
ASM has always been included in maven distribution. It was previously embedded in sisu and guice. We recently changed to use a standalone asm version. This was done to support JDK 21 ;-)
At least, now, we're in control and we don't have one or more repackaged/shaded version of ASM in the distribution.
So byte-buddy would be third consumer of ASM, not the first...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7662] Use proxies for session scoped beans [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #950:
URL: https://github.com/apache/maven/pull/950#discussion_r1386636863
##########
maven-core/pom.xml:
##########
@@ -144,6 +144,12 @@ under the License.
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
+ <dependency>
Review Comment:
Fwiw, in the distribution generated by this PR, I've made sure we use a byte-buddy version which does not embed ASM, so that we use a single version of it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on a diff in pull request #950: Use proxies for session scoped beans
Posted by GitBox <gi...@apache.org>.
laeubi commented on code in PR #950:
URL: https://github.com/apache/maven/pull/950#discussion_r1065459793
##########
maven-core/src/main/java/org/apache/maven/session/scope/internal/InstanceBuilder.java:
##########
@@ -0,0 +1,139 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.session.scope.internal;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Modifier;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.stream.Collectors;
+
+import com.google.common.base.Preconditions;
+import com.google.inject.Provider;
+import com.google.inject.internal.Errors;
+import com.google.inject.spi.Message;
+import net.bytebuddy.ByteBuddy;
+import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
+import net.bytebuddy.implementation.InvocationHandlerAdapter;
+import net.bytebuddy.matcher.ElementMatchers;
+
+/**
+ * Builds a proxy instance which is backed by a scoped provider.
+ *
+ * @author Simon Taddiken
+ * @param <T> Type of the proxy to create.
+ */
+final class InstanceBuilder<T> {
+
+ private Class<T> superType;
+ private InvocationHandler dispatcher;
+
+ private InstanceBuilder(Class<T> superType) {
+ this.superType = superType;
+ }
+
+ /**
+ * Builds a scoped proxy instance which is a subtype of the given class.
+ *
+ * @param type The class.
+ * @return Builder object for further configuration.
+ */
+ public static <E> InstanceBuilder<E> forType(Class<E> type) {
+ Preconditions.checkNotNull(type, "type");
+ Preconditions.checkArgument(
+ !Modifier.isFinal(type.getModifiers()), "can not proxy final type %s", type.getName());
+ return new InstanceBuilder<>(type);
+ }
+
+ /**
+ * Specifies the scoped provider. Every method call on the object created by this
+ * builder will be delegated to the object returned by the given provider.
+ *
+ * @param provider The scoped provider.
+ * @return Builder object for further configuration.
+ */
+ public InstanceBuilder<T> dispatchTo(Provider<T> provider) {
+ Preconditions.checkNotNull(provider, "provider");
+ final InvocationHandler callback = (proxy, method, args) -> method.invoke(provider.get(), args);
+ this.dispatcher = callback;
+ return this;
+ }
+
+ /**
+ * Creates the scoped proxy object using the given provider.
+ *
+ * @return The scoped proxy object.
+ */
+ @SuppressWarnings("unchecked")
+ public T create() {
+ Preconditions.checkState(this.dispatcher != null, "no provider set");
+ Class<? extends T> enhanced = new ByteBuddy()
+ .subclass(superType)
+ .method(ElementMatchers.any())
+ .intercept(InvocationHandlerAdapter.of(this.dispatcher))
+ .make()
+ .load(superType.getClassLoader(), ClassLoadingStrategy.Default.INJECTION)
+ .getLoaded();
+ Errors errors = new Errors();
+ T proxyInstance = createInstanceWithNullValues(enhanced, errors);
+ errors.throwProvisionExceptionIfErrorsExist();
+ return proxyInstance;
+ }
+
+ private <T> T createInstanceWithNullValues(Class<T> proxyClass, Errors errors) {
+ try {
+ final Constructor<T> ctor = getConstructorFor(proxyClass);
+ final Object[] args = new Object[ctor.getParameterCount()];
+ return callConstructor(ctor, errors, args);
+ } catch (final NoSuchMethodException e) {
+ errors.addMessage(new Message(e.getMessage(), e));
+ return null;
+ }
+ }
+
+ private <T> Constructor<T> getConstructorFor(Class<T> proxyClass) throws NoSuchMethodException {
+ final Collection<Constructor<?>> ctors = Arrays.stream(proxyClass.getConstructors())
Review Comment:
Not sure if it is required, but from my testing I have the impression that one either need an `@Inject` annotated constructor or a default contructor without arguments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7662] Use proxies for session scoped beans [maven]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #950:
URL: https://github.com/apache/maven/pull/950#discussion_r1387649317
##########
maven-core/pom.xml:
##########
@@ -144,6 +144,12 @@ under the License.
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
+ <dependency>
Review Comment:
Yes exactly, I think it is a case which becomes not rare to ignore.
About the interface thing it is mainly a matter of registering the scoped bean with an interface instead of its class so means `@SessionScoped(targetInterface = MyApi.class) class MyApiImpl implements Api {}` or something like that - once again we can relax it for internal usage but not for external ones - and `T` becomes `MyApi` instead of taking the actual instance.
Could it be a compromise to try to not shout in users foot?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7662] Use proxies for session scoped beans [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #950:
URL: https://github.com/apache/maven/pull/950#discussion_r1386635735
##########
maven-core/pom.xml:
##########
@@ -144,6 +144,12 @@ under the License.
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
+ <dependency>
Review Comment:
The choice to use a proxy is determined at runtime, depending if a bean to fulfil a given injection point is available or not. This would end up generating proxy classes for all injection points I think (unless we try to use some heuristic), maybe a bit too much.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7662] Use proxies for session scoped beans [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #950:
URL: https://github.com/apache/maven/pull/950#issuecomment-1803778150
Why is `@Typed` required? It has a serious side effect in Sisu regarding how bean is bound...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on a diff in pull request #950: Use proxies for session scoped beans
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #950:
URL: https://github.com/apache/maven/pull/950#discussion_r1065476794
##########
maven-core/pom.xml:
##########
@@ -144,6 +144,12 @@ under the License.
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
+ <dependency>
Review Comment:
since we already rely on build tools maybe let's just generate the proxy class at build time too and avoid to rely more on asm than the stack already does
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7662] Use proxies for session scoped beans [maven]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #950:
URL: https://github.com/apache/maven/pull/950#discussion_r1387070244
##########
maven-core/pom.xml:
##########
@@ -144,6 +144,12 @@ under the License.
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
+ <dependency>
Review Comment:
the new interceptor is a new proxy basically
high level i'd like to ensure we don't depend on java version as much as we can cause it starts to hurt all the ecosystem and at maven there is no reason we do depend on asm so no new usage should be added and ultimately we should drop them all from maven 4.0.0 IMHO
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7662] Use proxies for session scoped beans [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #950:
URL: https://github.com/apache/maven/pull/950#discussion_r1387041873
##########
maven-core/pom.xml:
##########
@@ -144,6 +144,12 @@ under the License.
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
+ <dependency>
Review Comment:
I'm not following. What are you worried about exactly ? ASM is already used, so it's just another usage. Related to class loaders, this PR does not change much, it simply adds byte buddy and objenesis to the maven core classloader, but ASM is already present, so there's actually no change related to ASM.
In the simplier form, I would have used a JDK `Proxy`, but this looks too limited, so I went directly with bytebuddy, but the effect is the same. It's transparent for the user.
I really don't get your worry as the classloader is nearly unchanged and proxies are only generated when actually needed (i.e. with the current plugins, it should never happen).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7662] Use proxies for session scoped beans [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #950:
URL: https://github.com/apache/maven/pull/950#discussion_r1387639811
##########
maven-core/pom.xml:
##########
@@ -144,6 +144,12 @@ under the License.
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
+ <dependency>
Review Comment:
> the new interceptor is a new proxy basically high level i'd like to ensure we don't depend on java version as much as we can cause it starts to hurt all the ecosystem and at maven there is no reason we do depend on asm so no new usage should be added and ultimately we should drop them all from maven 4.0.0 IMHO
Due to the way scoping works in guice, the interception is built by wrapping the injected bean, not by wrapping the injection point. The key method is the following:
https://google.github.io/guice/api-docs/4.2/javadoc/index.html?com/google/inject/Scope.html
```
public <T> Provider<T> scope(Key<T> key, Provider<T> unscoped);
```
So we do wrap the `Provider<T>`, but `T` is the actual bean class, because that happens irrespective of where the bean will be injected (the injection point).
So even if the bean is injected into a field which refers to an interface the bean implementation provider will be _scope_.
Basically, we can't use JDK proxies here.
I do understand your concerns about avoiding the use of ASM. We need to investigate, but it's actually used by sisu to discover beans. That sounds duplicate work as we usually generate the META-INF files so that discovery should not have to scan all classes.
That said, problems happen only when the following combination happens:
* the JDK in use is not supported by the ASM version embedded
* we scan a class file which targets a JDK newer that the one supported by ASM
That's why I've extracted ASM out of guice / sisu, so that we (or even the user) can easily upgrade the asm version in the distribution and get rid of those problems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7662] Use proxies for session scoped beans [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #950:
URL: https://github.com/apache/maven/pull/950#issuecomment-1803792327
Ah, true, yes, proxies are ONLY about interfaces.... kk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7662] Use proxies for session scoped beans [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet merged PR #950:
URL: https://github.com/apache/maven/pull/950
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7662] Use proxies for session scoped beans [maven]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #950:
URL: https://github.com/apache/maven/pull/950#discussion_r1386715402
##########
maven-core/pom.xml:
##########
@@ -144,6 +144,12 @@ under the License.
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
+ <dependency>
Review Comment:
well while asm is toggable it is fine (in several plugins) but if it becomes no more an option it also means a new release to use a new java version.
I'd like to avoid it.
Alternatively we can limit these scopes to interface injections where we don't need asm at all but ultimately we know upfront the types, the session scoped classes only so it is very generable at build time (like arc or openwebbeans do for ex).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a diff in pull request #950: Use proxies for session scoped beans
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #950:
URL: https://github.com/apache/maven/pull/950#discussion_r1065528079
##########
maven-core/src/main/java/org/apache/maven/session/scope/internal/InstanceBuilder.java:
##########
@@ -0,0 +1,139 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.session.scope.internal;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Modifier;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.stream.Collectors;
+
+import com.google.common.base.Preconditions;
+import com.google.inject.Provider;
+import com.google.inject.internal.Errors;
+import com.google.inject.spi.Message;
+import net.bytebuddy.ByteBuddy;
+import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
+import net.bytebuddy.implementation.InvocationHandlerAdapter;
+import net.bytebuddy.matcher.ElementMatchers;
+
+/**
+ * Builds a proxy instance which is backed by a scoped provider.
+ *
+ * @author Simon Taddiken
+ * @param <T> Type of the proxy to create.
+ */
+final class InstanceBuilder<T> {
+
+ private Class<T> superType;
+ private InvocationHandler dispatcher;
+
+ private InstanceBuilder(Class<T> superType) {
+ this.superType = superType;
+ }
+
+ /**
+ * Builds a scoped proxy instance which is a subtype of the given class.
+ *
+ * @param type The class.
+ * @return Builder object for further configuration.
+ */
+ public static <E> InstanceBuilder<E> forType(Class<E> type) {
+ Preconditions.checkNotNull(type, "type");
+ Preconditions.checkArgument(
+ !Modifier.isFinal(type.getModifiers()), "can not proxy final type %s", type.getName());
+ return new InstanceBuilder<>(type);
+ }
+
+ /**
+ * Specifies the scoped provider. Every method call on the object created by this
+ * builder will be delegated to the object returned by the given provider.
+ *
+ * @param provider The scoped provider.
+ * @return Builder object for further configuration.
+ */
+ public InstanceBuilder<T> dispatchTo(Provider<T> provider) {
+ Preconditions.checkNotNull(provider, "provider");
+ final InvocationHandler callback = (proxy, method, args) -> method.invoke(provider.get(), args);
+ this.dispatcher = callback;
+ return this;
+ }
+
+ /**
+ * Creates the scoped proxy object using the given provider.
+ *
+ * @return The scoped proxy object.
+ */
+ @SuppressWarnings("unchecked")
+ public T create() {
+ Preconditions.checkState(this.dispatcher != null, "no provider set");
+ Class<? extends T> enhanced = new ByteBuddy()
+ .subclass(superType)
+ .method(ElementMatchers.any())
+ .intercept(InvocationHandlerAdapter.of(this.dispatcher))
+ .make()
+ .load(superType.getClassLoader(), ClassLoadingStrategy.Default.INJECTION)
+ .getLoaded();
+ Errors errors = new Errors();
+ T proxyInstance = createInstanceWithNullValues(enhanced, errors);
+ errors.throwProvisionExceptionIfErrorsExist();
+ return proxyInstance;
+ }
+
+ private <T> T createInstanceWithNullValues(Class<T> proxyClass, Errors errors) {
+ try {
+ final Constructor<T> ctor = getConstructorFor(proxyClass);
+ final Object[] args = new Object[ctor.getParameterCount()];
+ return callConstructor(ctor, errors, args);
+ } catch (final NoSuchMethodException e) {
+ errors.addMessage(new Message(e.getMessage(), e));
+ return null;
+ }
+ }
+
+ private <T> Constructor<T> getConstructorFor(Class<T> proxyClass) throws NoSuchMethodException {
+ final Collection<Constructor<?>> ctors = Arrays.stream(proxyClass.getConstructors())
Review Comment:
I ended up using objenesis in the latest commit to create the instance so that the requirements on the original class are lessened.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on pull request #950: [MNG-7662] Use proxies for session scoped beans
Posted by GitBox <gi...@apache.org>.
laeubi commented on PR #950:
URL: https://github.com/apache/maven/pull/950#issuecomment-1385133727
@gnodet @cstamas @rmannibucau It would be great to finish this before 3.9.0 release! Seems a good opportunity for a minor release increment!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7662] Use proxies for session scoped beans [maven]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #950:
URL: https://github.com/apache/maven/pull/950#discussion_r1386957429
##########
maven-core/pom.xml:
##########
@@ -144,6 +144,12 @@ under the License.
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
+ <dependency>
Review Comment:
Well, this is not really the same, here you build an user facing feature (I'm thinking to extensions more than mojo) so it has to support this transparent java upgrade.
For "us" we don't care since our bytecode will always be consistent as you explained.
This is why I think we should generate the proxies at build time in out tooling stack more than at runtime - and I don't think doing it only for user code would be good.
this is my reasoning
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7662] Use proxies for session scoped beans [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #950:
URL: https://github.com/apache/maven/pull/950#discussion_r1387840616
##########
maven-core/pom.xml:
##########
@@ -144,6 +144,12 @@ under the License.
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
+ <dependency>
Review Comment:
> Yes exactly, I think it is a case which becomes not rare to ignore.
My point is that I mitigated the problem by externalising ASM. Whenever a new ASM release comes, we can easily bump it and release maven. But not sure it actually works, see the last point below.
> About the interface thing it is mainly a matter of registering the scoped bean with an interface instead of its class so means `@SessionScoped(targetInterface = MyApi.class) class MyApiImpl implements Api {}` or something like that - once again we can relax it for internal usage but not for external ones - and `T` becomes `MyApi` instead of taking the actual instance. Could it be a compromise to try to not shout in users foot?
I've reworked the PR in this way. The injection is limited to beans which have a `Typed` annotation (sisu or inject) and the proxies now use JDK proxies.
Related to sisu, I investigated a bit, and ASM is not used to actually discover classes, that's correctly done with the generated META-INF files, but it's used to discover the injection points without having to load the classes, see [sisu SpaceScanner](https://github.com/eclipse/sisu.inject/blob/master/org.eclipse.sisu.inject/src/main/java/org/eclipse/sisu/space/SpaceScanner.java).
Unfortunately, the sisu code does need [some small migration](https://github.com/eclipse/sisu.inject/commit/a9752361662edac045f16129a49e1a8b625aa604) when upgrading to a new ASM version to support reading new code...
Another point I just found is that the `java.lang.reflect.Proxy`... [is using ASM](https://github.com/openjdk/jdk/blob/dd9eab15c832c20e65681c21c5f91df11f4cddf9/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java#L56) to create the proxies ! So not sure we can really do much here... However, I assume the JDK uses an ASM version which support the JDK itself somehow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7662] Use proxies for session scoped beans [maven]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #950:
URL: https://github.com/apache/maven/pull/950#discussion_r1387851206
##########
maven-core/pom.xml:
##########
@@ -144,6 +144,12 @@ under the License.
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
+ <dependency>
Review Comment:
yes the JVM embed a relocated asm version which always works with their version, this is why I want to be able to guarantee your extension developped with jvm N+3 when maven was released with last jvm N works.
your change is very appreciated and ok for me, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org