Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inform the application about acknowledged and lost DATAGRAM frames #4273

Open
marten-seemann opened this issue Jan 26, 2024 · 5 comments
Open

Comments

@marten-seemann
Copy link
Member

marten-seemann commented Jan 26, 2024

It might be interesting to expose a notification to the application when a DATAGRAM frame is lost or acknowledged, similar to the FrameHandler interface. For example, we could change the method to SendDatagram(data []byte, onAcked, onLost func()) err error. This would give the application some hint what happened to a particular datagram. It would also be really simple to implement, since we just need to pipe through the FrameHandler callbacks to the user. We could then manually call the OnLost callback when we discard a DATAGRAM frame in the corner case described in (3). Note however, that this is just a hint, since:
* A packet might be declared lost even though it was actually received. This could happen if the ACK frame(s) for that packet are lost.
* Even if a packet was acknowledged, the DATAGRAM might not have been delivered to the application. All that the acknowledgment tells us is that the peer's QUIC stack received the packet, but depending on the implementation, delivery to the application might not be reliable.

Originally posted by @marten-seemann in #4145 (comment)

@marten-seemann
Copy link
Member Author

@chungthuang @joliveirinha @tanghaowillow @nanokatze Is that something that would be interesting for your use cases? Any thoughts on the proposed API?

@joliveirinha
Copy link
Contributor

yes. I believe it would be interesting to have this, even if just for the sake of having some custom "labelled" metrics on top of it.

@nanokatze
Copy link
Contributor

nanokatze commented Jan 29, 2024

It's not immediately clear whether it would be useful for my particular use case, I'd need to experiment to say.

Some things that pop up to me about the API are:

  • I guess you meant SendDatagram([]byte, onAcked, onLost func()) err error as opposed to returning onAcked, onLost func()? Otherwise I don't see how it can work at all
  • Assuming the above point, I was initially concerned about these callbacks being a subtle deadlock hazard (comparable to AllowConnectionWindowIncrease) but after inspecting the code, I don't think that's the case?

Beside these two, it would maybe make the API nicer if the user provided an interface { OnAcked(); OnLost() } object instead of two separate callbacks? But I don't really prefer either one over the other myself, just sharing some food for thought

@marten-seemann
Copy link
Member Author

  • I guess you meant SendDatagram([]byte, onAcked, onLost func()) err error as opposed to returning onAcked, onLost func()? Otherwise I don't see how it can work at all

Yes, you're right, thank you! I've correct the proposal.
This corrected version has the additional benefit that we don't need to allocate, or return anything allocated by quic-go (which would make pooling difficult) to the application.

  • Assuming the above point, I was initially concerned about these callbacks being a subtle deadlock hazard (comparable to AllowConnectionWindowIncrease) but after inspecting the code, I don't think that's the case?

Can you elaborate how this would be a deadlock? Obviously, the application must not block on these callbacks, or do any non-negligible amount of work, otherwise it would block quic-go's run loop.

Beside these two, it would maybe make the API nicer if the user provided an interface { OnAcked(); OnLost() } object instead of two separate callbacks? But I don't really prefer either one over the other myself, just sharing some food for thought

No strong opinion either way. Two function parameters seem fine to me, and it means we don't have to come up with a name for another interface 😉

@nanokatze
Copy link
Contributor

nanokatze commented Jan 29, 2024

Can you elaborate how this would be a deadlock? Obviously, the application must not block on these callbacks, or do any non-negligible amount of work, otherwise it would block quic-go's run loop.

What I was saying is that initially I wasn't sure whether it would deadlock (having thought that the callback could be called from inside some critical section that is also entered whenever the user calls some API.) I inspected the source and it indeed doesn't seem to, so please disregard that. I left that comment in because I wasn't fully certain in correctness of my findings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants