aioquic
aioquic copied to clipboard
Stream stop event
When we receive a stream stop we should propagate an event for client code. https://datatracker.ietf.org/doc/html/draft-ietf-quic-transport-29#page-127
Codecov Report
Merging #221 (c198e24) into main (9e360a5) will not change coverage. The diff coverage is
100.00%
.
@@ Coverage Diff @@
## main #221 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 23 23
Lines 4572 4579 +7
=========================================
+ Hits 4572 4579 +7
Impacted Files | Coverage Δ | |
---|---|---|
src/aioquic/quic/connection.py | 100.00% <100.00%> (ø) |
|
src/aioquic/quic/events.py | 100.00% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 9e360a5...c198e24. Read the comment docs.
Coverage and tests passing, not sure what to do about the package-wheel build error.
cibuildwheel
doesn't support versions under python3.6, however it's failing on python 3.10, could be a version matching bug.
@jlaine - It looks like the reason for the failure is that the check for python < 3.6
is incorrectly including 3.10
and therefore thinks it's a version lower than 3.6
.
Have you had a look at this PR otherwise? The code and coverage itself should be ready for review.
@jlaine Could I get some feedback on this PR please?
I've fixed CI, so you should be able to rebase your PR on top of main
I do think we need to expose this kind of event through the API. However I'm unsure about a couple of things:
- naming: neither the name StreamStop nor the doc strings correctly convey the fact that it's only the "sending" part (from our perspective) of the stream which has been stopped
- if we send a RESET_STREAM frame on a stream, do we want to be notified when it is ACK'd?
Hi @jlaine sorry for the delay, this has been on my todo list for quite a while. I've tried to address your feedback, let me know if there's anything else I may not have addressed.
I do think we need to expose this kind of event through the API. However I'm unsure about a couple of things:
- naming: neither the name StreamStop nor the doc strings correctly convey the fact that it's only the "sending" part (from our perspective) of the stream which has been stopped
I agree, the naming is not ideal, but nothing came to mind that was more obviously appropriate and also, I was trying as best I could to stick to existing conventions within the code. I've updated the docstring to try to clarify, but open to suggestions as well.
- if we send a RESET_STREAM frame on a stream, do we want to be notified when it is ACK'd?
This I'm not sure of, is it something that you feel should be taken care of in this PR? If so I would defer to your lead. (We haven't needed it in our use case)
@jlaine any thoughts on this?
My thoughts remain the same: that the API/naming are unclear. Unfortunately I don't have the mental bandwidth for a better proposal right now,..
@jlaine What about making an issue to address these questions further once you have the capacity, and meanwhile accepting this PR? We've been using it as part of our test infrastructure with the PR+latest master for over 7 months without any issue.
@jlaine What about making an issue to address these questions further once you have the capacity, and meanwhile accepting this PR? We've been using it as part of our test infrastructure with the PR+latest master for over 7 months without any issue.
Uhm no thanks. Code that gets merged adds to my maintenance burden, doubly so if it means backwards-incompatible changes down the line!
To be clear: I don't mean "I don't want this feature merged", I do think it definitely adds value. What I mean is "I don't want it merged half-baked". Please help me do the hard work of thinking it all the way through. Sorry about the tone of my previous message.
What about name StopSendingReceived ?
Superseeded by #393