Copilot commented on code in PR #12634:
URL: https://github.com/apache/maven/pull/12634#discussion_r3684570190
##########
impl/maven-core/src/main/java/org/apache/maven/DefaultMaven.java:
##########
@@ -221,9 +219,13 @@ private MavenExecutionResult
doExecute(MavenExecutionRequest request) {
sessionScope.seed(Session.class, session.getSession());
sessionScope.seed(InternalMavenSession.class,
InternalMavenSession.from(session.getSession()));
+ MavenSession previousSession = legacySupport.getSession();
legacySupport.setSession(session);
-
- return doExecute(request, session, result, chainedWorkspaceReader);
+ try {
+ return doExecute(request, session, result,
chainedWorkspaceReader);
Review Comment:
This change alters LegacySupport session scoping (save/restore instead of
unconditional clear). Given there are already unit tests for `DefaultMaven` in
this module, it would be good to add a regression test that verifies
`LegacySupport.getSession()` is restored correctly (e.g., nested/recursive
`Maven#execute()` calls or a pre-set session) to prevent future reintroductions
of session leaks.
##########
impl/maven-core/src/main/java/org/apache/maven/DefaultMaven.java:
##########
@@ -221,9 +219,13 @@ private MavenExecutionResult
doExecute(MavenExecutionRequest request) {
sessionScope.seed(Session.class, session.getSession());
sessionScope.seed(InternalMavenSession.class,
InternalMavenSession.from(session.getSession()));
+ MavenSession previousSession = legacySupport.getSession();
legacySupport.setSession(session);
-
- return doExecute(request, session, result, chainedWorkspaceReader);
+ try {
+ return doExecute(request, session, result,
chainedWorkspaceReader);
+ } finally {
+ legacySupport.setSession(previousSession);
Review Comment:
`legacySupport.setSession(session)` is executed before entering the
`try/finally`. If `setSession(session)` throws (e.g., due to OOME while
allocating the new `AtomicReference` in `DefaultLegacySupport#setSession`), the
previous session will not be restored. This became more important after
removing the outer `execute()`-level cleanup. Wrap the `setSession(session)`
call inside the same `try/finally` so restoration is guaranteed on any failure
after capturing `previousSession`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
