Run NIO close in the selector thread to prevent CME#381
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #381 +/- ##
============================================
- Coverage 66.25% 66.13% -0.12%
- Complexity 3021 3024 +3
============================================
Files 198 198
Lines 13519 13544 +25
Branches 2104 2109 +5
============================================
+ Hits 8957 8958 +1
- Misses 4015 4030 +15
- Partials 547 556 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -81,6 +89,10 @@ public static void close() { | |||
|
|
|||
| private static void close(boolean fromHook) { | |||
There was a problem hiding this comment.
Now, since the close() function is not actually closing the selector, can we rename this to either of markForClose(), markForShutdown(), scheduleClose(), etc. Close() is misleading!
Also, now since the logic for close is not very sequentially straightforward, possibly we can add a comment over the function what this function does and how selector will read the flag later to actually close it
There was a problem hiding this comment.
Now, since the close() function is not actually closing the selector, can we rename this to either of markForClose(), markForShutdown(), scheduleClose(), etc. Close() is misleading!
No, because close() is part of the public API and dnsjava follows SemVer.
What we could do is to remove the local* variables and close directly inside the lock. I don't quite see the benefit of this though, since calling selector() is possible again once close() has returned.
There was a problem hiding this comment.
Ahh yes! closer() is public!! Well then adding a javadoc would suffice!
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
While we here, possibly we can add a similar test in NioUdpClientTest too for completeness
There was a problem hiding this comment.
Feel free to create a PR once this one is merged.
There was a problem hiding this comment.
Sure, will do, works!
0cda51d to
4b6761f
Compare
878da0d to
6e700d8
Compare
|
Closes #379
Closes #380