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

Proposal: Cancellation #836

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Conversation

pbzweihander
Copy link
Contributor

Related issue: #528

Related pull request: #766

I made StopSource and StopToken types similar to the API described in https://github.com/yoshuawuyts/notes/blob/7ecb13a9999ae1fccd366ef0e246be5ca78867c0/rust/cancellation.md and used in tcp_listener and unix_listener modules.

But I'm not sure it actually 'gracefully' shuts down the server...

@jbr
Copy link
Member

jbr commented Jun 25, 2021

Is there a reason we wouldn't use stop-token (or stopper)?

Having implemented graceful shutdown for my other framework, it seems likely we'd need to integrate this fairly deeply into http-types and async-h1 as well

@pbzweihander
Copy link
Contributor Author

stopper seems great! I was worried that stop-token states in the documentation that it is 'Experimental'.

@jbr
Copy link
Member

jbr commented Jun 25, 2021

Two things that we'll want to address in addition to stopping the tcp stream: we will need to keep track of how many outstanding requests there are so we know when to shut down the server, and we will need to cut off keep alive connections. Currently, async-h1 will hold onto the tcp stream and wait for a subsequent request, which would delay shutdown by the keepalive timeout

@jbr
Copy link
Member

jbr commented Jun 25, 2021

As far as stop-token vs stopper, I don't have a great understanding of the current state of development of stop-token, which is part of why I wrote stopper. I also needed it to be runtime independent, but tide can depend on async-std.

We may very well discover bugs in stopper, but I'm committed to fixing them promptly

@pbzweihander
Copy link
Contributor Author

I made a draft PR in async-h1: http-rs/async-h1#188

And it seems working in this test. (It fails without the last commit)

@pbzweihander
Copy link
Contributor Author

I replaced vector of JoinHandles with waitgroup

@jbr
Copy link
Member

jbr commented Jun 28, 2021

Waitgroup looks a lot like how I did this in my other framework, with very minor differences (they're using an arc, I'm using an atomicusize which lets me keep track of how many outstanding requests there are): https://github.com/trillium-rs/trillium/blob/main/server-common/src/clone_counter.rs

I'd be totally fine copying mine over into tide, but waitgroup seems good and less code is certainly preferable

@laizy
Copy link
Contributor

laizy commented Jun 29, 2021

@jbr waitgroup author here, just for technical discussion. :)

they're using an arc, I'm using an atomicusize which lets me keep track of how many outstanding requests there are

  1. Actually, Arc's strong_count can track the number of outstanding tasks, so another atomicusize is not needed.
  2. Using Arc's strong_count has another benefit: waitgroup will be waked up only once when all outstanding tasks are finished, instead of waked up every time some task is finished.

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.

3 participants