Skip to content

Fix StaxEventItemWriter resource switching within a transaction #5177

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.

Sign up for GitHub

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

Jump to bottom
Closed
banseok1216 wants to merge 1 commit into from
Closed

Fix StaxEventItemWriter resource switching within a transaction #5177

banseok1216 wants to merge 1 commit into from
+354 −187

Conversation

@banseok1216
Copy link
Contributor

@banseok1216 banseok1216 commented Dec 20, 2025

Closes #5176

@banseok1216 banseok1216 force-pushed the fix/staxeventitemwriter-tx-resource-switch branch from a684ec6 to 9442a4e Compare December 20, 2025 13:37
@banseok1216
Fix StaxEventItemWriter resource switching within a transaction
4515a10 
Signed-off-by: banseok1216 <bansuk1216@naver.com>
@banseok1216 banseok1216 force-pushed the fix/staxeventitemwriter-tx-resource-switch branch from 9442a4e to 4515a10 Compare December 20, 2025 13:45
@fmbenhassine fmbenhassine mentioned this pull request Jan 21, 2026
Closed
@fmbenhassine
Copy link
Contributor

fmbenhassine commented Jan 21, 2026

Thank you for the PR!

I am just wondering, have you tried to make StaxEventItemWriter extend AbstractFileItemWriter to inherit that state management / resources binding "feature"? StaxEventItemWriter currently extends AbstractItemStreamItemWriter. I am asking because to me writing to a file (be it flat, json or xml) is the same thing, and currently only FlatFileItemWriter and JsonFileItemWriter inherit from AbstractFileItemWriter, so probably it's a good time to see if StaxEventItemWriter can/should join the hierarchy as well.

Probably this is not possible (due to previous design considerations which I need to check), in which case we can proceed with this PR which looks good to me.

@fmbenhassine fmbenhassine added the status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter label Jan 21, 2026
@fmbenhassine fmbenhassine mentioned this pull request Jan 21, 2026
Closed
@banseok1216
Copy link
Contributor Author

banseok1216 commented Jan 21, 2026

Thanks for the suggestion!

I did consider making StaxEventItemWriter extend AbstractFileItemWriter. However, my understanding is that the XML writing model in StaxEventItemWriter is quite different, and I’d need to spend more time reviewing AbstractFileItemWriter’s state/restart and open/close contract to see how well it would fit.

I initially tried to bring the implementations closer, but the changes quickly grew in scope. Given the potential side effects and compatibility concerns, I chose to keep this PR narrowly focused on fixing the immediate issue. 😅

@fmbenhassine
Copy link
Contributor

fmbenhassine commented Jan 21, 2026

Thank you for your feedback. That makes sense!

I will proceed with merging this PR for now and we can improve things if possible in a later iteration.

@fmbenhassine fmbenhassine added pr-for: bug in: infrastructure and removed status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter labels Jan 21, 2026
@fmbenhassine
Copy link
Contributor

fmbenhassine commented Jan 21, 2026

LGTM 👍 Rebased and merged as 5dc40a6.

Thanks again for looking into this and for contributing a fix!

jpraet
jpraet reviewed Feb 4, 2026
View reviewed changes
...ure/src/main/java/org/springframework/batch/infrastructure/item/xml/StaxEventItemWriter.java
* @throws XMLStreamException thrown if error occurs.
*/
@SuppressWarnings("DataFlowIssue")
protected void endDocument(XMLEventWriter writer) throws XMLStreamException {
Copy link
Contributor

@jpraet jpraet Feb 4, 2026

Choose a reason for hiding this comment

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

The removal of this protected method is a breaking change. I have a custom StaxEventItemWriter subclass that overrides this method.

Copy link
Contributor

@fmbenhassine fmbenhassine Feb 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

That's right, thank you for reporting this. I seem to have missed that when I reviewed the PR. I will reinstate that method in 6.0.3, see #5260.

Copy link
Contributor Author

@banseok1216 banseok1216 Feb 4, 2026

Choose a reason for hiding this comment

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

@jpraet @fmbenhassine I fixed this in #5261 — my mistake 😭

@fmbenhassine fmbenhassine mentioned this pull request Feb 4, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fmbenhassine fmbenhassine fmbenhassine left review comments

+1 more reviewer

@jpraet jpraet jpraet left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

in: infrastructure pr-for: bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ClosedChannelException and FileChannel leak when switching resources multiple times within the same transaction

3 participants

@banseok1216 @fmbenhassine @jpraet