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.

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

unify code that closes resources close() #1273

Open
vladopajic opened this issue Feb 28, 2025 · 2 comments
Open

unify code that closes resources close() #1273

vladopajic opened this issue Feb 28, 2025 · 2 comments

Comments

@vladopajic
Copy link
Member

vladopajic commented Feb 28, 2025

nim-libp2p does not have standardized method of dealing with common patter of closing occupied resources: connections, streams, etc...

this issue is opened with idea of coming up with approach for handling this common pattern. where some suggestion to begin with are in #1266 (comment) and code introduced in that PR.

agenda:

  • should code close or closeAndWait? when should code do one and when another?
  • should it be cancelable? when should code not do it?
  • check if code that close propagates unnecessary exceptions - like closing already closed resources can sometime raise exception. and these situations it might be better just not to raise it - closing already closed should have no effect
  • create utilities that deal with common boilerplate
  • apply decisions across code base
@kaiserd kaiserd moved this from new to Pipeline in nim-libp2p Feb 28, 2025
@arnetheduck
Copy link
Contributor

you will broadly be better off with close that does not raise exceptions - libp2p however has to deal with three forms of closing: "half-closing" which usually is known as shutdown, "full-closing" in response to the other side half-closing and abortively closing (resetting). The naming is unfortunate here, ie the name close is overloaded for all forms of closing.

it's the last one that should result in no exceptions and not be cancellable, similar to how closeWait is implemented on sockets for example.

Regarding cancellation of closing, again, this is one of the few exceptions where you want to catch cancellederror and ignore it. A close should always be non-blocking, ie it should do whatever it can to release resources and clean up, but any "in-progress" work (pending writes etc) can get lost if someone chooses to perform an irregular close (ie not using the above forms of half-closing and waiting for a response)

@arnetheduck
Copy link
Contributor

create utilities that deal with common boilerplate

my recommendation is to avoid too many of these - they tend to hide an underlying design problem - ie if you have to catch lots of exceptions in close, you're likely doing something wrong elsewhere and should have been reporting those errors in another way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Pipeline
Development

No branches or pull requests

2 participants