Fix concurrency issue from modifying the selectedKeysSet#141
Merged
kohlschuetter merged 1 commit intokohlschutter:mainfrom Oct 27, 2023
Merged
Conversation
This bug may have been a regression introduced in optimizing the key removal: kohlschutter@a7c3106 To reproduce refer to poc: https://github.com/kevink-sq/jetty-concurrency-issue-poc To verify fix, refer to poc branch (same as these changes): https://github.com/kevink-sq/jetty-concurrency-issue-poc/tree/kevink/attempted-junixsocket-fix-2 Jetty's [ManagedSelector](https://github.com/jetty/jetty.project/blob/7a7d69a69f4f51772e20813332291189a24e91b1/jetty-io/src/main/java/org/eclipse/jetty/io/ManagedSelector.java#L655) iterates through the selectedKeysSet and when separate processes cancel the AFSelectorKey, the iterator throws a `ConcurrentModificationException`. Fix is to introduce `cancelledKeysSet` to defer removal of cancelled keys until the select process. Use of the cancelled sets are referenced from java.nio.channels.spi's `AbstractSelector` and `AbstractSelectionKey`. Ideally we'd have `AFSelectionKey` extend from `AbstractSelectionKey` to leverage the base class's internal cancelled set but`AbstractSelectionKey` defines its own `isValid` and `cancel` and cannot override the base's final methods. Also added an atomic check to `AFSelectionKey#cancel` to prevent "Too many open files" socket exception.
Member
|
Thanks for the fix! 👍 |
Member
|
I just noticed that somehow this change breaks a test in junixsocket-tipc. Investigating... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This bug may have been a regression introduced in optimizing the key removal: a7c3106
To reproduce refer to poc: https://github.com/kevink-sq/jetty-concurrency-issue-poc
To verify fix, refer to poc branch (same as these changes): https://github.com/kevink-sq/jetty-concurrency-issue-poc/tree/kevink/attempted-junixsocket-fix-2
Jetty's ManagedSelector iterates through the selectedKeysSet and when separate processes cancel the AFSelectorKey, the iterator throws a
ConcurrentModificationException.Fix is to introduce
cancelledKeysSetto defer removal of cancelled keys until the select process. Use of the cancelled sets are referenced from java.nio.channels.spi'sAbstractSelectorandAbstractSelectionKey.Other socket implementation such as jnr-unixsocket extends the abstract classes (ref). Native Java sockets synchronize on the selectedSet (ref)
Ideally we'd have
AFSelectionKeyextend fromAbstractSelectionKeyto leverage the base class's internal cancelled set butAbstractSelectionKeydefines its ownisValidandcanceland cannot override the base's final methods.Also added an atomic check to
AFSelectionKey#cancelto prevent "Too many open files" socket exception.cc: @ThePumpingLemma who did most of the rca