[3.9.x] [MNG-7401] [MNG-7474] Keep a single maven session and fix session scope

[gnodet]

https://issues.apache.org/jira/browse/MNG-7401 https://issues.apache.org/jira/browse/MNG-7474

@laeubi this supersedes https://github.com/apache/maven/pull/666

[gnodet]

Not sure why tests aren't running. I'll close this one and recreate a new PR.

[gnodet]

Not sure why tests aren't running. I'll close this one and recreate a new PR.

Ah, closing and reopening did the trick.

[michael-o]

We have TLS-based solutions in past releases and we had to revert. What makes this one different?

[gnodet]

We have TLS-based solutions in past releases and we had to revert. What makes this one different?

It's not a new TLS, it's a move of the existing TLS from SessionScope to MavenSession. The problem I'm trying to solve is really MNG-7474 : the fact that session scoped beans are not singletons anymore, since the introduction of multiple sessions when introducing the concurrent builds. This is what introduced the TLS in the SessionScope, thereby breaking its purpose, i.e. beans are not cached within the session scope.

So there are several solutions to this problem, but I think the SessionScope is useless in the current form. So if we don't want to use a single session and keep things the way they are, I'd need to introduce a more global scope with a real singleton session object. We can't simply change the SessionScope, because beans are injected with the MavenSession, so if we want singletons, we need to have a consistent MavenSession object across mojo executions, which is not the case because of the multiple session clones. So the possibilities are:

• revert the use of multiple sessions, i.e. use a single session throughout the whole build, which is what this PR is about. The possible drawback is breakages of code that would use MavenSession.getCurrentProject() from a different thread
• introduce a new concept of singleton session + scope. if we want it immutable, it would basically be the MavenSession without the current project. The obvious drawback
• do nothing to fix the problem: this is a possibility and I'd have to work around the problem in m-build-cache-e by keeping state inside the MavenSession, or most probably in the RepositorySystemSession.getData() structure. In such a case, I think it would be better to deprecate the @SessionScope and related classes.

I think the cleaner state would be to fix the @SessionScope and use a singleton for MavenSession because that's semantically the most desirable state. Scopes are natural for DI, so it makes sense to use them, and the MavenSession cloning stuff has only been introduced to solve the getCurrentProject() problem. We should maybe also deprecate this method and suggest that plugins should have the project injected in a field. It would be difficult to get rid of it in the very short term, as that one is used a lot internally for the moment. We could also move it the LegacySupport along with the access to the current session.

[laeubi]

revert the use of multiple sessions, i.e. use a single session throughout the whole build, which is what this PR is about. The possible drawback is breakages of code that would use MavenSession.getCurrentProject() from a different thread

I think this is (at least for the Tycho scope of the problem) the most noteable thing, and we use LegacySupport what effectivly is a Threadlocal, so from what I can tell, in almost all cases MavenSession.getCurrentProject() returning the current project "of the thread" would be sufficient and cloning the session seem to be effectively for that purpose.

The problem with the cloning is, that if I have a sessionscoped componet I get the "rootsession" and thus calling MavenSession.getCurrentProject() most of the time returns null as the actual project is set on a cloned copy.

[laeubi]

We should maybe also deprecate this method and suggest that plugins should have the project injected in a field

I think the problem is that we then need some kind of "ProjectScope", I think MavenSession.set/getCurrentProject() should simply become a ThreadLocal, we document that and if one needs to pass data to a different thread he first need to fetch the current project.

[michael-o]

Is there a reasonable IT for this?

[gnodet]

I can write one for MNG-7474. MNG-7401 is an internal behavior and would require a junit test rather than an IT imho.

[michael-o]

I can write one for MNG-7474. MNG-7401 is an internal behavior and would require a junit test rather than an IT imho.

I guess so that would be best

[gnodet]

I can write one for MNG-7474. MNG-7401 is an internal behavior and would require a junit test rather than an IT imho.

I guess so that would be best

Unit test and IT added !