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

Class loading may fail when JVM is stopped using SIGINT #38154

Closed
wilkinsona opened this issue Oct 31, 2023 · 4 comments
Closed

Class loading may fail when JVM is stopped using SIGINT #38154

wilkinsona opened this issue Oct 31, 2023 · 4 comments
Labels
type: regression A regression from a previous release
Milestone

Comments

@wilkinsona
Copy link
Member

When the JVM is stopped using SIGINT, Boot's shutdown hook runs and closes the application context. Class loading that occurs during this close processing may fail due to a thread being interrupted while calling FileChannel.read(ByteBuffer, long). The interruption causes the read to close the channel and fail with a ClosedByInterruptException. This leaves FileChannelDataBlock with a FileChannel that should be open according to its reference count but isn't.

@wilkinsona wilkinsona added the type: regression A regression from a previous release label Oct 31, 2023
@wilkinsona wilkinsona modified the milestones: 3.2.0-RC2, 3.2.x Oct 31, 2023
@wilkinsona
Copy link
Member Author

@philwebb
Copy link
Member

philwebb commented Nov 1, 2023

The tracker stuff is only used in tests to I think we could get away without tracking the reopen if want to.

@wilkinsona
Copy link
Member Author

wilkinsona commented Nov 1, 2023

Thanks for having a look, Phil. I think I might be confused looking at the code in the future if we omit the tracking so I'd prefer to keep it. I've pushed a commit that refines the fix as I think it was a mistake to reset the thread's interrupted flag and retry the read. Instead, we now just repair the file channel and re-throw the exception. This allows the interruption on the current thread to continue will also ensuring that the file channel is usable on other threads. I've tested it with @onobc's sample that brought the problem to my attention and it seems to work.

@wilkinsona wilkinsona modified the milestones: 3.2.x, 3.2.0-RC2 Nov 1, 2023
@onobc
Copy link
Contributor

onobc commented Nov 1, 2023

Thanks for the quick turnaround @wilkinsona and @philwebb I will pickup the SNAPSHOT and verify it w/ Spring for Apache Pulsar.

wilkinsona added a commit to wilkinsona/spring-boot that referenced this issue Dec 7, 2023
In spring-projectsgh-38154, we started handling ClosedByInterruptException. The
FileChannel was repaired by recreating it and then the exception was
rethrown. This allowed other threads to use the channel that had been
read by an interrupted thread while allowing that interruption to
continue.

This approach has proven to be insufficient as there are scenarios
where the read needs to succeed on the interrupted thread. This
commit updates the handling of ClosedByInterruptException so that
this is the case. The FileChannel is recreated as before but the
thread's interrupted flag is now cleared before retrying the read.
The flag is then reinstated so that any subsequent actions that
should fail due to the interruption will do so.

We could clear and reinstate the interrupted flag before the first
read, rather than catching ClosedByInterruptException. This approach
was rejected as it will have an impact on the performance of the
happy path where the thread hasn't been interrupted.
philwebb pushed a commit that referenced this issue Dec 7, 2023
In gh-38154, we started handling ClosedByInterruptException. The
FileChannel was repaired by recreating it and then the exception was
rethrown. This allowed other threads to use the channel that had been
read by an interrupted thread while allowing that interruption to
continue.

This approach has proven to be insufficient as there are scenarios
where the read needs to succeed on the interrupted thread. This
commit updates the handling of ClosedByInterruptException so that
this is the case. The FileChannel is recreated as before but the
thread's interrupted flag is now cleared before retrying the read.
The flag is then reinstated so that any subsequent actions that
should fail due to the interruption will do so.

We could clear and reinstate the interrupted flag before the first
read, rather than catching ClosedByInterruptException. This approach
was rejected as it will have an impact on the performance of the
happy path where the thread hasn't been interrupted.

Fixes gh-38611
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: regression A regression from a previous release
Projects
None yet
Development

No branches or pull requests

3 participants