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

Better fix for scheduling logic for transport close() and abort() #2973

Merged
merged 4 commits into from
Jun 27, 2024

Conversation

ashleysommer
Copy link
Member

@ashleysommer ashleysommer commented Jun 26, 2024

Fixes #2921 #2890
Replaces #2966

Rather than simply calling abort() at a set interval after calling close(), we implement a mechanism to detect if any data is still in the transport write buffer after close is called, and schedules an async close operation to wait until the transport has finished, or finally calls abort after a reasonable timeout.

For more information, see my recent in-depth comments on #2921

This does not modify any existing Protocol transport logic (apart from the ill-placed abort()), does not introduce any breaking changes, and all tests pass without modification.

Rather than simply calling `abort()` at a set interval after calling `close()`, we implement a mechanism to detect if any data is still in the transport write buffer after close is called, and schedules an async close operation to wait until the transport has finished, or finally calls abort after a reasonable timeout.
@ashleysommer ashleysommer requested a review from a team as a code owner June 26, 2024 11:15
@ashleysommer
Copy link
Member Author

I think there needs to be additional communication (in the docs?) about ideally using a Sanic Streaming Response to chunk the response data when a response payload is larger than approx 16kb, rather than writing the whole payload in a single HTTP Response body. That would prevent the UVLoop UVStreamTransport from having situations where it cannot send the whole buffer in one go, so preventing this kind of error occurring that we were seeing here.

Though I know often its not possible to know the size of the response, eg, a dict passed to json() response could easily be a couple hundred KB without the user realizing it.

@ahopkins
Copy link
Member

Beautiful. Thanks for taking this on @ashleysommer. I wonder if we should add a configurable option to automatically kick over to streaming responses. The benefit to the dev is obviously not having to worry about sizing.

I'll give this a look later tonight and get it merged. We can backport to the LTS also.

@robd003
Copy link
Contributor

robd003 commented Jun 26, 2024

Thank you so much for taking the time to debug this @ashleysommer You're a rock star!

@ahopkins
Copy link
Member

I followed your logic and code thru. It is a great analysis. Thanks for taking the time to jump on this one @ashleysommer. 💪

ahopkins
ahopkins previously approved these changes Jun 27, 2024
@ahopkins ahopkins merged commit eafc178 into sanic-org:main Jun 27, 2024
25 checks passed
@ahopkins ahopkins mentioned this pull request Jun 27, 2024
@ashleysommer
Copy link
Member Author

I agree with the addition of the GRACEFUL_TCP_CLOSE_TIMEOUT. I was going to add that change to this PR, but I didn't want this diff to grow too large and affect other parts of sanic (I know adding new tuneables in the settings can be hard to get right). Thanks for adding it.

One further thing I would suggest, on startup during sanity-checks we could verify that GRACEFUL_TCP_CLOSE_TIMEOUT is less than GRACEFUL_SHUTDOWN_TIMEOUT, and issue a warning if it is not.
Or force:

GRACEFUL_TCP_CLOSE_TIMEOUT=min(GRACEFUL_SHUTDOWN_TIMEOUT, GRACEFUL_TCP_CLOSE_TIMEOUT)

@ahopkins
Copy link
Member

Forcing that would be awkward because it wouldn't behave as expected. Warning or error makes it clear. But I doubt it will be a value anyone will have any need to configure, so it's very unlikely we'd need this. Startup checks are however a good idea.

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.

Sanic drops part of HTTP response data
3 participants