Introduce "session_closed" handler callback #30945

yutakahirano · 2021-09-24T13:25:38Z

Introduce "session_closed" handler callback, and add support for
CLOSE_WEBTRANSPORT_SESSION capsule. There is no conforming
clients so it is not tested: I ran existing WPTs and confirmed
this didn't break them.

Also introduce WebTransportSession.dict_for_handlers to allow
handlers to put arbitrary data to the associated session. This is
different from Stash because Stash outlives sessions.

Introduce "session_closed" handler callback, and add support for CLOSE_WEBTRANSPORT_SESSION capsule. There is no conforming clients so it is not tested: I ran existing WPTs and confirmed this didn't break them. Also introduce WebTransportSession.dict_for_handlers to allow handlers to put arbitrary data to the associated session. This is different from Stash because Stash outlives sessions.

yutakahirano · 2021-09-24T13:30:33Z

This is related to aiortc/aioquic#229 a bit.

+@bashi for the overall change.
+@vasilvv for the logic in _h3_event_received and WebTransport.session. Am I using capsule correctly?

cc: @DavidSchinazi @nidhijaju FYI

bashi

Looks great! Some conforming questions.

tools/webtransport/h3/webtransport_h3_server.py

bashi · 2021-09-26T23:17:07Z

tools/webtransport/h3/webtransport_h3_server.py

@@ -225,6 +312,7 @@ def __init__(self, session: WebTransportSession,
                 callbacks: Dict[str, Any]) -> None:
        self._session = session
        self._callbacks = callbacks
+        self._allow_calling_session_closed = True


How about having _session_closed flag in WebTransportH3Protocol or WebTransportSession. It is set when close() is called. session_closed() handler is bypassed when the _session_closed flag is set.

(I would like to keep this handler class a thin wrapper of an actual handler. Putting state management in WebTransportH3Protocol or WebTransportSession will help keeping this wrapper simple)

_session_closed would need to be updated when session_closed is called (it's also called in quic_event_received). How would you update it?

For example, in WebTransportSession, wouldn't

self._session_closed = True in line 239

Add maybe_notify_session_closed() method. It calls self._handler.session_closed() only when self._session_closed = False

Use maybe_notify_session_closed() where we currently call self._handler.session_closed()
work?

How about the latest change?

webtransport/connect.any.js

bashi · 2021-09-27T03:45:24Z

tools/webtransport/h3/webtransport_h3_server.py

+        self.data = data
+
+    @staticmethod
+    def decode(data: bytes) -> any:


Just to confirm, Is the reason you use any instead of H3Capsule to work around type checking in Python3.6?

I'm using python3.8 but it complains at H3Capsule here.

I see. I thought PEP 563 should work for Python 3.8 but I forgot that we need to add from __future__ import annotations to enable it. I dropped it since it didn't work for Python 3.6. Thank you for clarification.

bashi · 2021-09-27T03:54:09Z

tools/webtransport/h3/webtransport_h3_server.py

@@ -225,6 +312,7 @@ def __init__(self, session: WebTransportSession,
                 callbacks: Dict[str, Any]) -> None:
        self._session = session
        self._callbacks = callbacks
+        self._allow_calling_session_closed = True


For example, in WebTransportSession, wouldn't

self._session_closed = True in line 239

Add maybe_notify_session_closed() method. It calls self._handler.session_closed() only when self._session_closed = False

Use maybe_notify_session_closed() where we currently call self._handler.session_closed()
work?

tools/webtransport/h3/webtransport_h3_server.py

bashi

LGTM, thank you!

bashi · 2021-09-27T07:17:01Z

tools/webtransport/h3/webtransport_h3_server.py

+        self.data = data
+
+    @staticmethod
+    def decode(data: bytes) -> any:


I see. I thought PEP 563 should work for Python 3.8 but I forgot that we need to add from __future__ import annotations to enable it. I dropped it since it didn't work for Python 3.6. Thank you for clarification.

bashi · 2021-09-27T07:28:41Z

tools/webtransport/h3/webtransport_h3_server.py

+        # TODO(yutakahirano): Implement the above.
+
+    def call_session_closed(self, close_info: Optional[Tuple[int, bytes]], abruptly: bool) -> None:
+        allow_calling_session_closed = self._allow_calling_session_closed


My mental model is that WebTransportSession has a state machine (e.g. not established, established, closed) and decides behavior based on the state so I slightly prefer to have an enum or _session_closed flag, but this approach will also work.

yutakahirano · 2021-09-27T13:06:36Z

@vasilvv can you take a look?

vasilvv

The overall approach looks good to me; I've not looked closely at the parsing code, as I assume that will change when you add support for multiple capsules.

yutakahirano · 2021-09-28T04:02:37Z

Thank you! I'm merging this.

Introduce "session_closed" handler callback, and add support for CLOSE_WEBTRANSPORT_SESSION capsule. There is no conforming clients so it is not tested: I ran existing WPTs and confirmed this didn't break them. Also introduce WebTransportSession.dict_for_handlers to allow handlers to put arbitrary data to the associated session. This is different from Stash because Stash outlives sessions.

wpt-pr-bot added infra webtransport wg-webtransport labels Sep 24, 2021

wpt-pr-bot assigned foolip Sep 24, 2021

wpt-pr-bot requested review from bashi, foolip and jgraham September 24, 2021 13:25

fix

d814385

wpt-pr-bot requested a review from nidhijaju September 24, 2021 15:01

bashi requested changes Sep 26, 2021

View reviewed changes

fix

1daca30

bashi requested changes Sep 27, 2021

View reviewed changes

fix

6b2517f

bashi approved these changes Sep 27, 2021

View reviewed changes

yutakahirano added 6 commits September 27, 2021 16:52

fix

b4039b5

ifx

3f0d7c9

fix

52c3caf

fix

f914743

fix

5850b34

fix

c2f0fa3

yutakahirano mentioned this pull request Sep 27, 2021

[WebTransport] Add support for multiples capsules #30982

Merged

vasilvv approved these changes Sep 28, 2021

View reviewed changes

yutakahirano merged commit 96b23e1 into master Sep 28, 2021

yutakahirano deleted the yhirano/webtransport-session-closed branch September 28, 2021 04:02

moz-wptsync-bot added mozilla:gecko-blocked and removed mozilla:gecko-blocked labels Sep 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce "session_closed" handler callback #30945

Introduce "session_closed" handler callback #30945

yutakahirano commented Sep 24, 2021

yutakahirano commented Sep 24, 2021

bashi left a comment

bashi Sep 26, 2021

yutakahirano Sep 27, 2021

bashi Sep 27, 2021

yutakahirano Sep 27, 2021

bashi Sep 27, 2021

yutakahirano Sep 27, 2021

bashi Sep 27, 2021

bashi Sep 27, 2021

bashi left a comment

bashi Sep 27, 2021

bashi Sep 27, 2021

yutakahirano commented Sep 27, 2021

vasilvv left a comment

yutakahirano commented Sep 28, 2021

Introduce "session_closed" handler callback #30945

Introduce "session_closed" handler callback #30945

Conversation

yutakahirano commented Sep 24, 2021

yutakahirano commented Sep 24, 2021

bashi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bashi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yutakahirano commented Sep 27, 2021

vasilvv left a comment

Choose a reason for hiding this comment

yutakahirano commented Sep 28, 2021