python-trio / python-trio/trio
[discussion] should we set SO_LINGER when ungracefully closing a socket?
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 7.3k
- Forks
- 431
- Avg merge
- 2d 17h
- Merged PRs (30d)
- 6
Description
Streams, and presumably soon Listeners, implement the trio.abc.AsyncResource interface:
def forceful_close(self):
"""Close immediately, even if this causes the shutdown to be "unclean"."""
async def graceful_close(self):
"""Close gracefully, blocking if necessary.
SPECIAL CASE: if this is cancelled or otherwise raises an error, then the underlying resource is still
closed (possibly in an "unclean" way).
"""
Then there's trio sockets which just have a sync close method, which is fine because socket closure isn't a blocking operation (outside of some really obscure and useless cases that you have to opt into... well, modulo this silly bug? I think?), and the point of the trio.socket API is to directly expose all the quirky low-level details of sockets.
And finally there's async file objects (trio.open_file, trio.wrap_file), which have an async close method, because the underlying OS APIs just don't provide any truly non-blocking or forceful close for disk files. This close method has the same semantics as graceful_close: it blocks and executes a checkpoint, but if cancelled then it's still guaranteed to close the underlying object.
So I was thinking... giving streams and files different APIs for closing is confusing, especially since they're basically doing the same thing. So maybe we should make async file objects implement the AsyncResource API.
This would mean implementing a forceful_close that occasionally blocks the event loop for a little while (boo), but if you really need to get this thing closed immediately and you to do it from synchronous context, it will do that. And the graceful_close mechanism of course works fine.
But then I was thinking... given that implementation of FileWrapper.forceful_close and FileWrapper.graceful_close, you actually would never want to call forceful_close. The reason forceful_close exists is when you need to close something but can't afford to block indefinitely, e.g. because you have been cancelled. But if you think about it, really, the AsyncResource API provides three mechanisms for closing: there's the two above, and then there's:
async def async_forceful_close(resource):
with trio.cancel_scope() as cs:
cs.cancel()
await resource.graceful_close()
Because async_forceful_close runs in a cancelled context, it doesn't block (at least, not any more than the absolute minimum), but it's still a checkpoint and async-colored. And for FileWrapper this does the right thing, which sync forceful_close doesn't.
You can also imagine a resource that absolutely does need to block momentarily during even an unclean shutdown, like perhaps a database connection in an app where they decide as a matter of local policy that it should get that chance to shut down. Since forceful_close is sync-colored, it can't block at all no matter what (except by blocking the main loop).
And finally... there are not many contexts where it makes sense to call forceful_close directly. AsyncResource objects don't provide with, just async with. Internally, wrappers like SSLStream.graceful_close will call self.transport_stream.forceful_close() when they're on the path where things have failed and they need to do an unclean shutdown before re-raising, but they could use async_forceful_close too (with a tiny cost in efficiency, I guess). Our server helper will probably want to wrap each connection task in a last-ditch stream close operation:
async def wrapped_connection_task(stream, afn):
try:
await afn(stream)
finally:
stream.forceful_close()
and this can't use graceful_close because it runs outside of any timeout control. But it could just as easily use something like the async_forceful_close incantation.
(Or maybe it should just let the descriptor leak, so that users see ResourceWarning?)
Anyway... it's really feeling like TOOWTDI is telling us to dump forceful_close, at least in its current form.
Concerns:
- I don't want to rename
graceful_closeto justclose, that's confusing given Python's strong existing convention ofclosebeing a sync method - The
with open_cancel_scope() as cs: cs.cancel(); stream.graceful_close()incantation is super obscure. - It is pretty handy when implementing things like
SSLStream.graceful_closeto have a nice way to say "welp I give up and this shutdown is going to be dirty" and propagate that to the underlying stream
Some options:
-
Keep
AsyncResource.forceful_close, make it async, give it a default implementation using the cancel-scope incantation, so from the caller's point of view it's parallel tograceful_close, and I guess subclasses can override if they want to for some tiny efficiency gains. (I guess we could even make bothforceful_closeandgraceful_closehave default implementations in terms of the other, and tell people to implement whichever one makes sense for their type? Unfortunately stockabc.ABCdoes not have a way to say "subclass must implement one or the other of these two methods". Also this is a lot of subclassy cleverness which I dislike.) -
Since
graceful_closehas to implement both types of behavior anyway, make itgraceful_close(can_block=True). Maybe change the name too, sincegraceful_close(can_block=False)is not actually graceful. But then we're still putting an extra burden on implementors: they have to implement any special handling for cancellation/errors, and they have to check for this bool, and they could potentially get out of sync. -
Provide a
trio.forceful_closefunction that does the cancellation dance. It's not parallel tograceful_close(global function versus method), but most people never need to think about it anyway.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the AsyncResource interface and the Stream, socket, and async file closing APIs described in the issue. Compare the proposed closing semantics and determine a consistent API; done means the design is settled and the affected implementations and tests are identified, though this issue names no files or tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- networking
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100