Introduce LocalRestConnectionClosed exception #117
Conversation
Codecov Report
@@ Coverage Diff @@
## v0.x.x #117 +/- ##
==========================================
+ Coverage 96.25% 96.31% +0.06%
==========================================
Files 5 5
Lines 1255 1276 +21
==========================================
+ Hits 1208 1229 +21
Misses 47 47
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
I just realized I can catch it in the Timer routine itself. One second.. |
AndrejMitrovic
force-pushed
the
byebye-node
branch
from
5c0b782 to
af4b139
Compare
|
I made it better now. The |
This allows tests to gracefully handle peer nodes shutting down.
AndrejMitrovic
force-pushed
the
byebye-node
branch
from
af4b139 to
3da4383
Compare
|
|
||
| size_t count; // up to 3 seconds wait | ||
| while (!atomicLoad(connection_closed)) | ||
| assert(count < 300); count++; Thread.sleep(10.msecs); |
There was a problem hiding this comment.
So this doesnt need curly braces? D is a weird language.
There was a problem hiding this comment.
You know what happened? The stupid coverage bot failed my PR because 3 lines weren't covered. So I put them all on the same line. And then I removed the braces. I need sleep.
|
So, is this an issue with client code, or LocalRest ? I'm quite wary of introducing Exceptions that can be handled for specific LocalRest situation. |
IIUC, with this we are almost silently ignoring closed connections. I feel like it should be handled at the client side, but not sure if it is possible to do in agora since we target both LocalRest and vibe.d. |
We can catch it explicitly and throw a different exception. It will again lead to the node crashing in the tests. But anyway we need some way of shutting down all nodes at once.. |
|
Maybe some way of notifying all schedulers to ignore connection failures first, and then issuing the shutdown command.. |
|
What about a watchdog in the main thread? We can check to see if any node dies prematurely. |
This will allow as to at least handle node shutdowns gracefully when using LocalRest in our test-suite. At least it should fix crashes in the tests involving periodic timers.