Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upOnly select for tcp write when there is something to write #96
Conversation
|
Thanks for the investigation! I'll try to look at this asap. I assume with this changes your scenario is now working correctly? |
| @@ -265,8 +248,10 @@ private void processWrite() { | |||
| c.bind(local); | |||
| } | |||
|
|
|||
| final ChannelState channelState = new ChannelState(c); | |||
| c.register(selector, SelectionKey.OP_CONNECT, channelState); | |||
ibauersachs
Mar 17, 2020
Member
AFAIR this won't work in a multithreaded scenario. A registration on the selector must be made on the selector thread or it can cause deadlocks.
AFAIR this won't work in a multithreaded scenario. A registration on the selector must be made on the selector thread or it can cause deadlocks.
rouzwawi
Mar 17, 2020
Author
I guess you're referring to this in the Selector docs:
A selector's key and selected-key sets are not, in general, safe for use by multiple concurrent threads. If such a thread might modify one of these sets directly then access should be controlled by synchronizing on the set itself. The iterators returned by these sets' iterator methods are fail-fast: If the set is modified after the iterator is created, in any way except by invoking the iterator's own remove method, then a ConcurrentModificationException will be thrown.
Since SelectableChannel.register modifies the selectors key set, it might cause that ConcurrentModificationException to be thrown. But in general it seems that the register method itself can be called from multiple threads.
Anyhow, I think I will revert back to using the registration queue to avoid having to deal with concurrent modification exceptions on the selector thread.
I guess you're referring to this in the Selector docs:
A selector's key and selected-key sets are not, in general, safe for use by multiple concurrent threads. If such a thread might modify one of these sets directly then access should be controlled by synchronizing on the set itself. The iterators returned by these sets' iterator methods are fail-fast: If the set is modified after the iterator is created, in any way except by invoking the iterator's own remove method, then a ConcurrentModificationException will be thrown.
Since SelectableChannel.register modifies the selectors key set, it might cause that ConcurrentModificationException to be thrown. But in general it seems that the register method itself can be called from multiple threads.
Anyhow, I think I will revert back to using the registration queue to avoid having to deal with concurrent modification exceptions on the selector thread.
ibauersachs
Mar 17, 2020
Member
It's been a while, but I think I actually ran into deadlocks (not the ConcurrentModificationException) when registering from outside the selector thread.
It's been a while, but I think I actually ran into deadlocks (not the ConcurrentModificationException) when registering from outside the selector thread.
rouzwawi
Mar 17, 2020
Author
fixed
fixed
|
Any ETA on when this can be merged/released? |
Yes I’ve tested a local snapshot build with a service and i don’t see the timeouts (and 100% cpu core usage). |
Done. It might take a while until it has propagated through Central. |
|
Great thanks! |
Avoids putting the selector loop in full spin by removing the
OP_WRITEselection key when there's nothing to write. I also removed theregistrationQueuefrom the tcp client since we can just call connect immediately as we create the socket, and register theOP_CONNECTkey there.fixes #95