Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle graceful shutdown on TransportUseClosedError. #1065

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Handle graceful shutdown on TransportUseClosedError. #1065

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e45c6ed
Handle graceful shutdown on TransportUseClosedError.
AlejandroCabeza Mar 11, 2024
453bcc6
Update with PR suggestions.
AlejandroCabeza Mar 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions libp2p/switch.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,11 @@ proc upgradeMonitor(
finally:
upgrades.release()

proc acceptCleanup(upgrades: AsyncSemaphore, connection: Connection) {.async.} =
upgrades.release() # always release the slot
if not isNil(connection):
await connection.close()

proc accept(s: Switch, transport: Transport) {.async.} = # noraises
## switch accept loop, ran for every transport
##
Expand Down Expand Up @@ -263,11 +268,13 @@ proc accept(s: Switch, transport: Transport) {.async.} = # noraises
trace "releasing semaphore on cancellation"
upgrades.release() # always release the slot
return
except TransportUseClosedError:
trace "Graceful shutdown in accept loop, exiting"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can use the same as below "Exception in accept loop, exiting", exc = exc.msg

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you think that might trigger the same reactions, from users, that @cheatfate was mentioning in the issue?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue was about the error log level. We can keep the one you used, my comment was about the rest.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@diegomrsantos diegomrsantos Mar 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cheatfate

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue might be fixed by #1095 (comment). I'm tempted to close this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaiserd

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we test if it actually has been fixed? If yes, let's close this PR and link 1095 in the close message.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment, I don't know how to reproduce it, if it hasn't been fixed yet.

await acceptCleanup(upgrades, conn)
return
except CatchableError as exc:
error "Exception in accept loop, exiting", exc = exc.msg
upgrades.release() # always release the slot
if not isNil(conn):
await conn.close()
await acceptCleanup(upgrades, conn)
return

proc stop*(s: Switch) {.async, public.} =
Expand Down
Loading