[net] internal/http3: add net/http.ResponseController support
Nicholas Husin (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil added 4 comments![]( "Open in Gerrit")
writeCtx, writeCancel := context.WithCancelCause(context.Background())Need to cancel these when the stream is closed.
return nilThis can be just `return context.Cause(ds.ctx)`, which returns nil if the context is not canceled.
if err := st.readDeadline.err(); err != nil {Add a comment mentioning that we're checking the deadline before potentially writing to a stream buffer? (In other places as well.)
It's not obvious why we can't just call Read or Write and let it fail when the deadline has passed.
But I wonder if this would all be simpler if we handled deadlines by half-closing the stream (CloseRead/CloseWrite) on deadline expiry. Then we don't need to check anything here other than checking after a read/write error to see if we need to map it into a deadline error.
(I believe half-closing the stream will still let fast-path reads/writes succeed, but that's fine since the fast-path never blocks and will revert to the slow path as soon as the buffer fills.)
return 0, errIf err is io.EOF, this should still be errH3FrameError (truncated varint).
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin added 4 comments![]( "Open in Gerrit")
writeCtx, writeCancel := context.WithCancelCause(context.Background())Need to cancel these when the stream is closed.
Right, thanks! Done.
(We still have some usage like `st.stream.stream.CloseWrite` which bypasses the context cancelation elsewhere in the codebase, but only in tests. I'll send it as a separate CL to keep the diff clearer here unless you prefer otherwise.)
This can be just `return context.Cause(ds.ctx)`, which returns nil if the context is not canceled.
Done (for posterity, I still kept the `err` method rather than inlining it for clarity).
Add a comment mentioning that we're checking the deadline before potentially writing to a stream buffer? (In other places as well.)
It's not obvious why we can't just call Read or Write and let it fail when the deadline has passed.
But I wonder if this would all be simpler if we handled deadlines by half-closing the stream (CloseRead/CloseWrite) on deadline expiry. Then we don't need to check anything here other than checking after a read/write error to see if we need to map it into a deadline error.
(I believe half-closing the stream will still let fast-path reads/writes succeed, but that's fine since the fast-path never blocks and will revert to the slow path as soon as the buffer fills.)
Added comments for now. I think immediate failures regardless of buffer is probably a less confusing behavior to have.
Also, I might be missing something, but I think half-closing the stream makes it so the client thinks that a server's response is successful rather than cancelled, since the server will send a `FIN` rather than a `RESET_STREAM` to the client. We can make it so that when the timer expires, we send a `RESET_STREAM`, but that also introduces complications like making the client discard buffered data that was received prior to the deadline exceeding I think.
If err is io.EOF, this should still be errH3FrameError (truncated varint).
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil voted and added 2 comments![]( "Open in Gerrit")
Votes added by Damien Neil
| Code-Review | +2 |
2 comments
writeCtx, writeCancel := context.WithCancelCause(context.Background())Nicholas HusinNeed to cancel these when the stream is closed.
Right, thanks! Done.
(We still have some usage like `st.stream.stream.CloseWrite` which bypasses the context cancelation elsewhere in the codebase, but only in tests. I'll send it as a separate CL to keep the diff clearer here unless you prefer otherwise.)
Acknowledged
if err := st.readDeadline.err(); err != nil {Nicholas HusinAdd a comment mentioning that we're checking the deadline before potentially writing to a stream buffer? (In other places as well.)
It's not obvious why we can't just call Read or Write and let it fail when the deadline has passed.
But I wonder if this would all be simpler if we handled deadlines by half-closing the stream (CloseRead/CloseWrite) on deadline expiry. Then we don't need to check anything here other than checking after a read/write error to see if we need to map it into a deadline error.
(I believe half-closing the stream will still let fast-path reads/writes succeed, but that's fine since the fast-path never blocks and will revert to the slow path as soon as the buffer fills.)
Added comments for now. I think immediate failures regardless of buffer is probably a less confusing behavior to have.
Also, I might be missing something, but I think half-closing the stream makes it so the client thinks that a server's response is successful rather than cancelled, since the server will send a `FIN` rather than a `RESET_STREAM` to the client. We can make it so that when the timer expires, we send a `RESET_STREAM`, but that also introduces complications like making the client discard buffered data that was received prior to the deadline exceeding I think.
Current approach seems okay if you prefer it, but I think this would be simpler without the contexts.
I think the simpler approach would be two time.Timers, one each for read and write deadline. Timer is created lazily at the time of the first Set{Read,Write}Deadline call.
Read timer calls quic.Stream.CloseRead, which interrupts any reads in progress.
Write timer calls quic.Stream.Reset(H3_REQUEST_CANCELLED) (https://www.rfc-editor.org/info/rfc9114/#section-8.1-2.26.1), which interrupts any writes in progress and sends a RESET_STREAM to the peer.
When a body read or write encounters an error, it checks for an expired timeout and returns a timeout error if appropriate. (We assume that any error encountered was caused by the timeout.)
Cleanup stops the timers, if they were created.
I think this almost entirely keeps the timeout behavior local to Set{Read,Write}Deadline, except for the need to check for an expired timeout to return the right error.
If we really feel that an expired timeout should cause an error on an operation which can be handled by the fast-path buffers, we can make the quic.Stream fast-path return an error when the stream is (half-)closed; the fast-path buffers are guarded by mutexes now, so CloseRead/CloseWrite can easily set some field guarded by the mutex to indicate closure.
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |