Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.14%. Comparing base (e41d1f9) to head (7758b02).
Report is 1 commits behind head on master.

@@            Coverage Diff             @@
##           master    #4523      +/-   ##
==========================================
- Coverage   85.18%   85.14%   -0.04%     
==========================================
  Files         154      154              
  Lines       14802    14794       -8     
==========================================
- Hits        12608    12595      -13     
- Misses       1687     1691       +4     
- Partials      507      508       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Hi @GeorgeMac! Thank you for putting in the effort to debug this! Is there any way to properly test this, either as a unit or an end-to-end test. This seems very subtle, and I'm concerned we'll break this again in the future.

I'm a bit confused about my own code now:

1. It seems unnecessary to do state tracking in the datagrammer. Since the sendErr and the receiveErr are only set from the outside, it seems like we could just remove the isDone return value on SetReceiveError and SetSendError, right?
2. I'm wondering if we should replace the onStateChange callback with two callbacks, one for closing the send and one for closing the receive side. This would allow us to get rid of the streamState enum altogether.

What do you think?

Hi @GeorgeMac! Thank you for putting in the effort to debug this! Is there any way to properly test this, either as a unit or an end-to-end test. This seems very subtle, and I'm concerned we'll break this again in the future.

I'm a bit confused about my own code now:

It seems unnecessary to do state tracking in the datagrammer. Since the sendErr and the receiveErr are only set from the outside, it seems like we could just remove the isDone return value on SetReceiveError and SetSendError, right?
I'm wondering if we should replace the onStateChange callback with two callbacks, one for closing the send and one for closing the receive side. This would allow us to get rid of the streamState enum altogether.

What do you think?

I see both of what you're saying there @marten-seemann 👍

Correct me if I am wrong (slowly catching up), the purpose of the state tracking system is twofold:

1. Free the datagrammer when both send and recv are closed.
2. Proxy closing of send and receive cancelling / errors from the stream onto the datagrammer (by way of onStateChange triggering Set(Send|Receive)Error)?

What you're suggesting makes sense. Another idea could be to remove the callbacks in favour of a couple interfaces to represent the datagrammer and removal of the datagrammer from the parent connection. Something like:

type streamCloser interface {
  streamClose(quic.StreamID)
}

type sendRecieveError interface {
    SetSendError(error)
    SetRecieveError(error)
}

type stateTrackingStream struct {
    quic.Stream
    
    sendError error
    receiveError error
    
    errorer sendRecieveError
    closer streamCloser
}

Where the purpose of the stateTrackingStream becomes to continue proxying, but the moment the pair of send and recv error are non-nil, it calls s.closer.streamClose(s.StreamID()).

streamClose would be implemented on connection and would clean the entry from the map?

Happy to take a stab at whichever approach makes sense and write tests around it.

Correct me if I am wrong (slowly catching up), the purpose of the state tracking system is twofold:

1. Free the datagrammer when both send and recv are closed.
2. Proxy closing of send and receive cancelling / errors from the stream onto the datagrammer (by way of onStateChange triggering Set(Send|Receive)Error)?

Correct!

Your idea works as well. No strong preference either way. I usually find callbacks a tiny bit easier to test, but the difference is quite minimal.

@marten-seemann I pushed up a take on what I described. See what you think. Happy to adjust or go back to callbacks.

Thank you @MarcoPolo 🙏

Thanks for the great feedback!