Handle graceful shutdown on TransportUseClosedError. #1065
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1065 +/- ##
============================================
- Coverage 82.81% 82.53% -0.28%
============================================
Files 91 91
Lines 15750 15822 +72
============================================
+ Hits 13043 13059 +16
- Misses 2707 2763 +56
|
That's a good idea, but it might be a tad too hard for me: Otherwise, this might be too time-consuming (for me). |
| @@ -273,11 +278,13 @@ proc accept(s: Switch, transport: Transport) {.async.} = # noraises | |||
| except CancelledError as exc: | |||
| trace "releasing semaphore on cancellation" | |||
| upgrades.release() # always release the slot | |||
| except TransportUseClosedError: | |||
| trace "Graceful shutdown in accept loop, exiting" | |||
There was a problem hiding this comment.
we can use the same as below "Exception in accept loop, exiting", exc = exc.msg
There was a problem hiding this comment.
Don't you think that might trigger the same reactions, from users, that @cheatfate was mentioning in the issue?
There was a problem hiding this comment.
I think the issue was about the error log level. We can keep the one you used, my comment was about the rest.
There was a problem hiding this comment.
It seems to me that it makes sense for the others to still spit error, as long as it's an actual error. The issue, as I understood, is it was logging it as an error something that wasn't an actual error, hence confusing people.
There was a problem hiding this comment.
Users aren't supposed to use trace unless they are trying to debug an issue. That was precisely my intention suggesting "Exception in accept loop, exiting", exc = exc.msg, adding more useful info to the log, especially the exception msg.
There was a problem hiding this comment.
There was a problem hiding this comment.
The issue might be fixed by #1095 (comment). I'm tempted to close this PR.
There was a problem hiding this comment.
Can we test if it actually has been fixed? If yes, let's close this PR and link 1095 in the close message.
There was a problem hiding this comment.
At the moment, I don't know how to reproduce it, if it hasn't been fixed yet.
|
If testing it is too time consuming, or not even possible, that's fine. |
|
What is the status here? |
diegomrsantos
force-pushed
the
fix/issue/1007-graceful-shutdown
branch
from
ceabad6
to
453bcc6
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1065 +/- ##
==========================================
- Coverage 84.53% 84.53% -0.01%
==========================================
Files 91 92 +1
Lines 15517 16414 +897
==========================================
+ Hits 13118 13875 +757
- Misses 2399 2539 +140
|
fixes: #1007