aioquic icon indicating copy to clipboard operation
aioquic copied to clipboard

Stream stop event

Open silverbucket opened this issue 2 years ago • 14 comments

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

silverbucket avatar Aug 31 '21 11:08 silverbucket

Codecov Report

Merging #221 (c198e24) into main (9e360a5) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            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.

codecov[bot] avatar Aug 31 '21 11:08 codecov[bot]

Coverage and tests passing, not sure what to do about the package-wheel build error.

silverbucket avatar Aug 31 '21 11:08 silverbucket

cibuildwheel doesn't support versions under python3.6, however it's failing on python 3.10, could be a version matching bug.

silverbucket avatar Sep 02 '21 09:09 silverbucket

@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.

silverbucket avatar Sep 21 '21 09:09 silverbucket

@jlaine Could I get some feedback on this PR please?

silverbucket avatar Oct 27 '21 16:10 silverbucket

I've fixed CI, so you should be able to rebase your PR on top of main

jlaine avatar Oct 27 '21 21:10 jlaine

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?

jlaine avatar Oct 27 '21 21:10 jlaine

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)

silverbucket avatar Jan 18 '22 19:01 silverbucket

@jlaine any thoughts on this?

silverbucket avatar Feb 09 '22 09:02 silverbucket

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 avatar Feb 09 '22 23:02 jlaine

@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.

silverbucket avatar Apr 04 '22 14:04 silverbucket

@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!

jlaine avatar Apr 04 '22 21:04 jlaine

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.

jlaine avatar Apr 05 '22 04:04 jlaine

What about name StopSendingReceived ?

vranem1 avatar May 03 '22 14:05 vranem1

Superseeded by #393

jlaine avatar Nov 04 '23 15:11 jlaine