[MNG-7455] Use a single session object during the whole build

[gnodet]

Attempt to fix https://issues.apache.org/jira/browse/MNG-7455 without reverting https://issues.apache.org/jira/browse/MNG-7347 (SessionScoped beans should be singletons for a given session).
#714 is the same targeted at maven-3.9.x branch.
This is also similar to #666 (fix http://issues.apache.org/jira/browse/MNG-7401).

If we want to fix the original problem which is that components annotated with @SessionScoped should be shared in a given session, we need to get rid of the thread local in the SessionScope, which has been done in #621. There will be no concurrency issue as the enter/exit methods will only be called once for the session.

But we have 2 conflicting facts: @SessionScoped beans should be shared during the execution of the reactor, and we have cloned sessions. I think cloned sessions was just a way to fix some problems with the MT build, so i’d rather get rid of that one. Also I think projects embedding maven such as the eclipse stuff and mvnd do reuse the container for multiple sessions and will leverage the @SessionScoped more that what we have now if that’s possible.

Because the session scope provides a clear boundary between stateless objects (or at least those that can be reused across builds) and those that needs to be discarded. Caches, depending on what they cache, may belong to one or the other category.

[michael-o]

Uuuh, we have already burned ourselves with TLS. Is this really necessary?

[gnodet]

Uuuh, we have already burned ourselves with TLS. Is this really necessary?

I'd be happy to find another solution, but the revert would introduce back a TLS anyway, without actually fixing the original problem.

[gnodet]

Uuuh, we have already burned ourselves with TLS. Is this really necessary?

I'd be happy to find another solution, but the revert would introduce back a TLS anyway, without actually fixing the original problem.

One possibility, but not a trivial fix, would be to deprecate MavenSession#getCurrentProject() and create a new interface that for the execution context which would hold the session, the current project and the current mojo execution.

[rmannibucau]

Not a big fan if this InheritableThreadLocal we know doesn't work.
Cant we already create a new API and keep the session instances? Maybe a bit like in your immutable PR but without any TL?
A ClassLoader based map replacing the TL is likely better since it avoids the thread binding issue but it still does not cover all cases since a mojo regularly creates a classloader with a parent which is the jvm one but it is easier to instrument these cases in the classrealm to track them IMHO so sounds like an option TL free, no?

[gnodet]

Not a big fan if this InheritableThreadLocal we know doesn't work.
Cant we already create a new API and keep the session instances? Maybe a bit like in your immutable PR but without any TL?
A ClassLoader based map replacing the TL is likely better since it avoids the thread binding issue but it still does not cover all cases since a mojo regularly creates a classloader with a parent which is the jvm one but it is easier to instrument these cases in the classrealm to track them IMHO so sounds like an option TL free, no?

So reverting will introduce back the TLS, see https://github.com/apache/maven/blob/maven-3.8.4/maven-core/src/main/java/org/apache/maven/session/scope/internal/SessionScope.java#L70
In order to restore compatibility, we'll need to introduce one TLS somewhere I think. Using class loaders won't work as classloaders are usually the same for projects in a reactor when no extensions are used.

We could revert MNG-7347 on 3.8.x and provide a cleaner fix for 3.9.x only though.

There are several options to cleanly fix the problem:

• deprecate MavenSession#getCurrentProject() and introduce a new ExecutionContext holding the MavenSession, MavenProject and MojoExecution that would be passed as a parameter in various components and could be injected in the mojo, but that means a lot of changes in the various interfaces, so a big incompatibility
• introduce a new @BuildScoped which would behave the way we want, i.e. share components for the duration of the reactor build. That would definitely be easier, but we'd leave the TLS in the SessionScope and the somewhat broken state.
• a third alternative would be to just fix the SessionScope to cache beans for the duration of the whole build, always injecting the root session

[rmannibucau]

@gnodet do I get it right the revert would be a revert of the revert so get us back to the state which was not working? Think we'll need to use a ClassFileTransformer in ClassRealm to be able to hack the ThreadFactories created by components to handle that properly and it would avoid to track the threads and associated projects for most cases. For extensions it is a bit harder but AFAIK the issue mainly pops up with mojo which can start background tasks using the project so maybe we should just fix it for mojo and use a TL for all other cases (but never for a mojo ClassRealm, this is why I spoke of a classloader). Could it be a compromise working for something close to all cases?

[gnodet]

Closing this PR in favour of #715 on 3.8.x branch and keep #714 for 3.9.x branch.

[gnodet]

@gnodet do I get it right the revert would be a revert of the revert so get us back to the state which was not working? Think we'll need to use a ClassFileTransformer in ClassRealm to be able to hack the ThreadFactories created by components to handle that properly and it would avoid to track the threads and associated projects for most cases. For extensions it is a bit harder but AFAIK the issue mainly pops up with mojo which can start background tasks using the project so maybe we should just fix it for mojo and use a TL for all other cases (but never for a mojo ClassRealm, this is why I spoke of a classloader). Could it be a compromise working for something close to all cases?

I don't think using ClassLoader to discriminate would work, as the same classloader can be reused when building all modules if none of them define any extensions.

[rmannibucau]

@gnodet alone you are right but with the 3 menrionned hacks it works. Only thing we are qure as of today is TL or ITL does not work so we must find something else I fear.

[gnodet]

@gnodet alone you are right but with the 3 menrionned hacks it works. Only thing we are qure as of today is TL or ITL does not work so we must find something else I fear.

I'm really not following. The problem raised by 7347 is that components annotated with @SessionScope should be reused during the duration of the session (i.e. the reactor build). That was not the case (and I also found out the fix is not sufficient I think). So we don't really want to discriminate components, it's really the opposite, they should be shared. So it's really independent of any classloading mechanism imho.