Add concurrency test for processReadyKeys() to detect potential thread-safety issues#380
Add concurrency test for processReadyKeys() to detect potential thread-safety issues#380bhaveshthakker wants to merge 3 commits intodnsjava:masterfrom
Conversation
…nt.processReadyKeys
…y copying selectedKeys
|
Thanks for the demo. I played around with it, and the only way I can see that the modification of the keys can be triggered outside of the selector thread is by the Is this the situation you encountered this originally, or is there some other way this can be reproduced without the Please also note:
|
|
Yes, close() is the only place from issue seems to be reproducible! Regarding the earlier fix (bhaveshthakker#1): that was created while I was experimenting with the failure, and I ended up pushing it prematurely. It wasn't intended as a finalized PR, so please feel free to disregard it. Regarding unit test, yes, it was written just to reproduce the issue Waiting for the solution to come in. LMK if I can be of any help |
Closes #379 Closes #380 Co-authored-by: bhaveshthakker <[email protected]>
|
Does the linked PR #381 work for you? |
Closes #379 Closes #380 Co-authored-by: bhaveshthakker <[email protected]>
Closes #379 Closes #380 Co-authored-by: bhaveshthakker <[email protected]>
Closes dnsjava#379 Closes dnsjava#380 Co-authored-by: bhaveshthakker <[email protected]>
2 participants
This PR introduces a unit test that verifies the thread-safety of NioClient.processReadyKeys() under concurrent channel registration and readiness.
The test assumes correct behavior — i.e., no ConcurrentModificationException should be thrown when new channels become ready while processReadyKeys() is iterating over selector.selectedKeys(). If such an exception is thrown, the test fails, indicating a concurrency flaw in the current implementation.
Key points:
Uses a real selector loop via NioClient.selector(), with no changes to production logic.
Concurrently registers channels and makes them ready using Pipe.sink().write() to trigger select() updates.
Catches ConcurrentModificationException using a custom Thread.UncaughtExceptionHandler.
Includes processing delay to increase overlap and expose the race condition.
| Note: As this is a concurrency test, it may not fail consistently across runs or environments. Flakiness is expected until the underlying issue is fixed.
This test is intended as a reproducible regression check for an existing bug and will serve as a validation gate once a thread-safe fix is applied to processReadyKeys().