Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce feature flag for auto-closing AutoCloseable in Jupiter's ExtensionContext.Store #4442

Closed
wants to merge 5 commits into from

Conversation

YongGoose
Copy link
Contributor

@YongGoose YongGoose commented Apr 2, 2025

Overview

fix #4434


I hereby agree to the terms of the JUnit Contributor License Agreement.


Definition of Done

- Suppress deprecation warnings for `CloseableResource` usage
- Ensure proper handling of `InterruptedException` in resource closing
- Refactor code to avoid deprecated API usage where possible
@YongGoose
Copy link
Contributor Author

YongGoose commented Apr 2, 2025

@marcphilipp

Treat AutoCloseable like CloseableResource in Jupiter's stores when configuration parameter is on (true)

Do you think we should add a test case for this scenario? 🤔

Also, I'm wondering whether the purpose of this PR is solely to add configuration parameter or if it should implement methods to handle AutoCloseable as CloseableResource.

*/
void close() throws Throwable;
void close() throws Exception;
Copy link
Member

Choose a reason for hiding this comment

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

Changing this would be binary-incompatible. Thus, I don't think we can have CloseableResource extend AutoCloseable after all. I guess we could support both CloseableResource and AutoCloseable instead as separate interfaces. I'll discuss this with the team.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you.
Please discuss this with the team and share the outcome! 👍🏻

Copy link
Member

Choose a reason for hiding this comment

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

@YongGoose We've updated the issue description in #4434.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it!
I will close this PR, start a new task, and then create a new PR.

Also, if you have some time, could you briefly explain how the discussion went on the issue below? 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Also, if you have some time, could you briefly explain how the discussion went on the issue below? 🙂

I just posted an update: #1767 (comment)

@YongGoose YongGoose closed this Apr 6, 2025
@YongGoose YongGoose deleted the feature/4434 branch April 6, 2025 06:57
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.

Introduce feature flag for auto-closing AutoCloseable in Jupiter's ExtensionContext.Store
2 participants