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

Improve Node.js server shutdown and error handling #2554

Merged
merged 3 commits into from
Oct 23, 2021

Conversation

armanbilge
Copy link
Member

Just some minor tweaking to make the code a bit nicer.

More importantly, I think this suffers from same/similar problems as #2300, namely: how do you gracefully shutdown/handle errors? The critical thing seems to be that any sockets remaining in the queue should be destroyed.

Perhaps I'm missing something, but it seems to me that the problem lies with the server method. With the serverResource method, I can conceptualize the "listening" part of the server being tied to the resource, so that even after the resource is closed the stream of sockets is still usable. But maybe this is leaky?

cc @RaasAhsan

@mpilquist
Copy link
Member

@armanbilge Does this PR change the behavior to match the JVM version?

@armanbilge
Copy link
Member Author

Hmm, a bit fuzzy since I made this. It makes two changes:

  1. The use of Async instead of a Deferred to handle errors at server creation. This should just be code cleanup.
  2. Use a None-terminated Queue of Sockets (the previous was unterminated). A None is added to the queue when the server resource is closed.

What is still an open problem is how to guarantee that all the sockets are destroyed. Right now, if they are unconsumed by the Stream, they will sit open in the queue indefinitely.

Does this PR change the behavior to match the JVM version?

Not sure, what exactly does the JVM side do?

@armanbilge armanbilge changed the title Tweak Node.js server to be more idiomatic Improve Node.js server shutdown and error handling Oct 23, 2021
@mpilquist mpilquist merged commit 0f20c99 into typelevel:main Oct 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants