Skip to content

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Nov 8, 2023

And make it used in new HTTP/2 transports.

Note about implementation:

  • code base integrating Resolver 2.x (like Maven) SHOULD switch to new session handling
  • code base using resolver w/o managing it (like Maven Mojos are) does NOT have to move to new session handling (basically resolver for them work unchanged).

In short, the Resolver 1.x session handling is still present, but is deprecated (DefaultRepositorySystemSession default ctor) and will emit warnings if application integrating Resolver 2.x still uses Resolver 1.x session handling and is about to use new Resolver 2.x features (like HTTP/2 transport) that require onSessionClose. Doing that will produce leaks, unless they are one-time CLI apps.


https://issues.apache.org/jira/browse/MRESOLVER-302

*
* @since TBD
*/
interface CloseableRepositorySystemSession extends RepositorySystemSession, Closeable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not making the RepositorySystemSession always implement Closeable with an empty default method close()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because that will suddenly emit warning in all the existing code (think Mojos) that "using closeable reasource without try-with-resource" or alike. Closing session is really objective of "resolver managing" client code (like Maven), but "resolver using (but not managing it)" client code (like Maven Mojos) should really not be annoyed is session closeable or not, they should NEVER close it anyway,

@cstamas cstamas marked this pull request as ready for review November 8, 2023 18:03
@cstamas cstamas added this to the 2.0.0 milestone Nov 8, 2023
@cstamas cstamas requested review from gnodet and kwin November 8, 2023 20:38
Copy link
Contributor

@gnodet gnodet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cstamas cstamas merged commit a6faf6d into apache:master Nov 9, 2023
@cstamas cstamas deleted the MRESOLVER-302-takeTwo branch November 9, 2023 10:22
@jira-importer
Copy link

Resolve #974

@jira-importer
Copy link

Resolve #974

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants