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

Attempt to close all delegate readers even when some fail #4764

Conversation

elimelec
Copy link
Contributor

@elimelec elimelec commented Feb 20, 2025

Motivation:

When a delegate reader fails during close(), the original implementation would stop immediately, preventing other delegates from completing their cleanup.

Similar to #4750

Signed-off-by: Elimelec Burghelea <elimelec1@protonmail.com>
Signed-off-by: Elimelec Burghelea <elimelec1@protonmail.com>
@elimelec
Copy link
Contributor Author

elimelec commented Feb 20, 2025

@fmbenhassine This is split into 2 commits:

  • One for CompositeItemReader
  • One for CompositeItemStream

I think this PR (and also the previous one: #4750) may change the public API. I realized only now because some tests were failing because of the changes from CompositeItemStream. Also, the documentation was explicit that the close method is not fully cleaning all delegates.

I still left both changes since the interfaces is defined with throws ItemStreamException and I am not changing that part.

Let me know if I should change something.

@fmbenhassine
Copy link
Contributor

Thank you for this second PR! Appreciated 👍

I see no public API changes, but I agree there is a small behavioral change in the sense that the exception is now suppressed instead of being the main one. Since we are fixing the partial cleanup bug, I believe this is acceptable for a patch release as long as we document the change about the way the exception is thrown (which is missing by now, but I will take care of that).

The PR LGTM, I will plan it for 5.2.2.

@fmbenhassine fmbenhassine added this to the 5.2.2 milestone Feb 24, 2025
fmbenhassine added a commit that referenced this pull request Feb 24, 2025
@fmbenhassine
Copy link
Contributor

Rebased and merged, thank you for your contributions!

As mentioned previously, I updated the javadocs about the change in suppressed exceptions: 75edb29.

@elimelec elimelec deleted the close-all-delegate-readers-on-close branch February 25, 2025 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants