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

Add connection_closed or connection_dropped event #43

Closed
rmarx opened this issue Jan 21, 2020 · 3 comments
Closed

Add connection_closed or connection_dropped event #43

rmarx opened this issue Jan 21, 2020 · 3 comments

Comments

@rmarx
Copy link
Contributor

rmarx commented Jan 21, 2020

Currently, we rely on packet_* with a CONNECTION_CLOSE frame, but that's not always enough. E.g., the server can decide to drop a connection after a long timeout without sending a CONNECTION_CLOSE. Or, we might want additional information of when a connection is effectively dropped completely (according to @marten-seemann: is supposed to happen 3 PTOs after it is retired)

Maybe a connection_closed event with a trigger field suffices? Should this be importance Base or Extra?

@rmarx
Copy link
Contributor Author

rmarx commented Nov 1, 2020

So I'm currently breaking my head on this one as there are a lot of aspects to closing connections in QUIC:

  1. there are three different states: closing, draining, actually closed
  2. there are many different reasons for "closing": idle timeout/handshake timeout, "immediate close", stateless reset, no overlapping versions, ...
  3. within immediate close, there are different error spaces: connection errors, application errors (which don't always reflect the actual internal error properly)

Currently (see draft02 branch), these things are kind of spread out across three different events:

  • connectivity:connection_state_updated
  • generic:connection_error
  • generic:application_error

Combining this into a single connection_closed event with all that flexibility would thus lead to some serious duplication across the board..., not to mention a very complex event

However, looking at https://github.com/lucas-clemente/quic-go/pull/2501/files, which seems the main use case for @marten-seemann at least, all he really cares about is logging a free-form string "reason" for a connection_closed event, as well as having a separate event type so it's easy to query just the connection_closed, instead of having to sift through connection_state_updated.

So, my proposal would be to add connection_closed as a quite high-level event, mainly useful for manual interpretation, referring to the other existing events if the need arises to log more fine-grained info.

Thus, the proposed design of connection_closed for draft-02:

{
        owner?:"local"|"remote",

        connection_error_code?:uint32,
        application_error_code?:uint32,
     
        reason?:string
}

Triggers:
* clean
* handshake_timeout
* idle_timeout
* error // basically the "immediate close" case
* stateless_reset
* version_mismatch

(reminder, in qlog, each event has an implicit trigger field, which can contain any string value, so other triggers would be valid here as well)

I'm not particularly happy with this design, but it's better than others I could come up with.

Does this suit your use case @marten-seemann? Can you let me know by tomorrow evening (2nd November)? Thanks!

@marten-seemann
Copy link
Collaborator

Why do we need generic:connection_error and a generic:application_error?

rmarx pushed a commit that referenced this issue Nov 2, 2020
- Closes #43. Related to #85, #78, and #49
@rmarx
Copy link
Contributor Author

rmarx commented Nov 2, 2020

Merged connection_error and application_error with connection_closed, as that indeed makes more sense here.

Closing for now, open to revisiting this for draft-03.

@rmarx rmarx closed this as completed Nov 2, 2020
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

2 participants