Raise EOF when writing to a closed sandbox over the network #2330
Conversation
modal/io_streams.py
Outdated
| ) | ||
| except GRPCError as exc: | ||
| if exc.status == Status.FAILED_PRECONDITION: | ||
| raise EOFError("Stdin is closed. Cannot write to it.") |
There was a problem hiding this comment.
Can't you grab the message in the gRPC exception? It'd be more accurate and forward compatible.
Also not sure EOFError is what a user should expect here, could we instead throw an exception from within the modal.exception namespace that is like 'invalid error'?
Also re-reading the docstring I don't like that it suggests that drain() may always send EOF. It'd be nice to reword that to indicate EOF is only sent once the writer is closed.
There was a problem hiding this comment.
Fixed the comments and switched to ValueError to match python native socket behavior.
pawalt
force-pushed
the
pawalt/io_closed
branch
2 times, most recently
from
69768ac to
6a1fad6
Compare
|
@thundergolfer I added a test |
pawalt
force-pushed
the
pawalt/io_closed
branch
from
6a1fad6 to
f5b6d1b
Compare
modal/io_streams.py
Outdated
| ) | ||
| except GRPCError as exc: | ||
| if exc.status == Status.FAILED_PRECONDITION: | ||
| raise ValueError("I/O operation on closed file.") |
There was a problem hiding this comment.
Would still prefer if the client relied on the message from the server because in the future there could be multiple ways of receiving a FAILED_PRECONDITION.
There was a problem hiding this comment.
Wait can I convince you otherwise on this - I want to match the exact error that Python throws on close FDs for close API compatibility, but I don't think it makes sense for the RPC to throw this. I think the separation is valuable here, for the same reason that we don't throw FAILED_PRECONDITION back to the user.
pawalt
force-pushed
the
pawalt/io_closed
branch
3 times, most recently
from
2b35347 to
b846ece
Compare
|
@prbot automerge |
![modal-pr-review-automation[bot]](https://avatars.githubusercontent.com/in/335990?s=60&v=4){kind=small}
This feature works again.
pawalt
force-pushed
the
pawalt/io_closed
branch
from
b846ece to
dcfe25b
Compare
Describe your changes
If Stdin is closed (particularly relevant for sandboxes), and EOF error will be throw instead of accepting the write.
Backward/forward compatibility checks
Check these boxes or delete any item (or this section) if not relevant for this PR.
Note on protobuf: protobuf message changes in one place may have impact to
multiple entities (client, server, worker, database). See points above.
Changelog
When writing to a
StreamWriterthat has already had EOF written, a ValueError is now raised instead of anEOFError.