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

The WebSocketStream Interface #48

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open

The WebSocketStream Interface #48

wants to merge 33 commits into from

Conversation

ricea
Copy link
Collaborator

@ricea ricea commented Jul 14, 2023

Specify the WebSocketStream API. This exposes backpressure to
JavaScript, avoiding pathological behavior when one side of the
connection is not keeping up. It also provides a modern API using
Streams and Promises to interoperate cleanly with the modern web
ecosystem.

See the explainer at
https://github.com/ricea/websocketstream-explainer/blob/master/README.md
for examples and motivation.

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

Copy link

@Lamzin Lamzin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Adam,

Thanks for the PR! Looks good in general. However, I left a few comments.

Thanks,
Oleh

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
@ricea
Copy link
Collaborator Author

ricea commented Jan 28, 2024

The algorithms are complete now.

@nidhijaju @domenic PTAL

The PR Preview is unhappy for some reason, but it builds locally.

@ricea ricea changed the title [WIP] The WebSocketStream Interface The WebSocketStream Interface Jan 28, 2024
@ricea ricea removed the do not merge yet Pull request must not be merged per rationale in comment label Jan 28, 2024
Temporarily force-link "writable stream writer" until it can be exported
from the streams standard.
This feature is not worth the complexity so it is being removed.
…cancel()"

Put back mention of passing close code or reason to abort() or cancel(),
in preparation for updating it to use WebSocketError.
Some algorithms that are common to WebSocket and WebSocketStream have
been moved to a new section to be shared.
@dead-claudia
Copy link

Quick question: wouldn't it be better to model it this way?

  • ws = new WebSocketStream(url, options?): Open WebSocket
  • void await ws.ready: Wait for ready, fails with a OperationError DOMException on premature close.
  • void await ws.closed: Wait for closure
  • ws.readable: Receive messages like now, reads fail with a NotReadableError DOMException on close.
  • ws.writable: Send messages like now, writes fail with a NetworkError DOMException on close. Can be written to before the connection is established, to allow for writing messages immediately upon open.
  • To close, just close the writer. The current values of closeCode and closeReason are used for the socket close frame.
  • On server close, sets closeCode and closeReason accordingly.

WebIDL:

dictionary WebSocketStreamOptions {
  sequence<USVString> protocols;
  AbortSignal signal;
};

interface WebSocketStream {
  constructor(USVString url,
              optional WebSocketStreamOptions options);
  readonly attribute ReadableStream readable;
  readonly attribute WritableStream writable;
  readonly attribute sequence<DOMString> extensions;
  readonly attribute DOMString protocol;
  readonly attribute Promise<undefined> opened;
  readonly attribute Promise<undefined> closed;
  [Clamp] attribute unsigned short closeCode;
  attribute USVString closeReason;
};

This has a few other added benefits as well:

  1. It's closer to fetch and its response = await fetch(request) processing model. Notably, you can write to requests in fetch before the request is even issued.
  2. Everything just references a shared object of a single type. Easier for developers to wrap their minds around and easier for implementors to work with.
  3. No need for a dedicated close method, and custom close codes and reasons even work with pipeThrough.

Note: I dropped the URL field. Userland code can just employ a Map if they truly need to care about it.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@annevk annevk added the needs implementer interest Moving the issue forward requires implementers to express interest label Jan 29, 2024
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only got a little ways through before I had to wrap up for the day.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Also apply backpressure on connection, and linkify "exists".
@ricea
Copy link
Collaborator Author

ricea commented Jan 30, 2024

  • ws.readable: Receive messages like now, reads fail with a NotReadableError DOMException on close.

I think always throwing an exception on close would lead to poor ergonomics. For example, even a simple function like

const wss = new WebSocketStream('/messages');
for await (const message of wss.readable) {
  const div = new HTMLDivElement();
  div.textContent = message;
  body.appendChild(div);
}

would have to be wrapped in a try { ... } catch { ... } block to deal with the NotReadableError exception.

  • ws.writable: Send messages like now, writes fail with a NetworkError DOMException on close. Can be written to before the connection is established, to allow for writing messages immediately upon open.

This is what WebTransport does with datagrams, but it adds a lot of complexity to the standard and implementation. In the case of WebSockets, no messages can be sent until the handshake response has been received anyway, so there's no performance benefit in queuing them early. I consciously chose not to give out the streams until they were useful to avoid the need to think about these issues.

  • To close, just close the writer. The current values of closeCode and closeReason are used for the socket close frame.

This means you have to pass around the writer to any place that needs to be able to close the WebSocket. I have the impression that most users of WebSockets don't bother with the close codes but for those who do it feels awkward:

wss.closeCode = 1000;
wss.closeReason = 'Thank you';
writer.close();
  • On server close, sets closeCode and closeReason accordingly.

I'm not comfortable that they can be changed asynchronously outside of script:

  1. If their timing was unlucky, a developer could overwrite the value sent by the server when trying to close the connection from the user agent, and then there'd be no way to recover it.
  2. closeCode and closeReason will change before any reaction to wss.closed is executed, meaning that script could synchronously observe the fact the connection was closed before it had been informed. This sort of racy behaviour leads to hard-to-understand bugs.

This has a few other added benefits as well:

  1. It's closer to fetch and its response = await fetch(request) processing model. Notably, you can write to requests in fetch before the request is even issued.

In the case of HTTP, it's necessary to send the request body first, so it's not an exact parallel. fetch() doesn't make the response body available for reading until it actually has a response, which I think is the right choice.

  1. Everything just references a shared object of a single type. Easier for developers to wrap their minds around and easier for implementors to work with.

It's a bit easier to implement, yes, but in my opinion it's harder to use. Developers won't have to think about the various dictionaries in my design, they can just copy the calling patterns from examples.

Implementing the construction of a Request from a RequestInit to match the fetch standard is a nightmare, but from the developer's perspective it's just an option bag, it doesn't even require thought.

WebSocketError is a bit of a wart, but I suspect most developers will never have to construct one.

  1. No need for a dedicated close method, and custom close codes and reasons even work with pipeThrough.

You can actually pass a WebSocketError through pipeThrough, but I doubt anyone will.

Note: I dropped the URL field. Userland code can just employ a Map if they truly need to care about it.

I'm not sure of the usefulness of the URL field, but it's very easy to implement, and maybe if it saves someone from needing to use a Map it is worth it.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't yet get to the last two sections, but it's all looking quite nice so far; no substantial comments.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
ricea and others added 3 commits January 31, 2024 15:14
Co-authored-by: Domenic Denicola <[email protected]>
ricea and others added 6 commits January 31, 2024 15:17
Co-authored-by: Domenic Denicola <[email protected]>
Also use <p class=note> everywhere, and other cleanups.
Make those headings be indented under "The WebSocket interface" for a
more logical organisation.

Also move "Ping and Pong frames" section to the bottom as it applies to
both APIs.
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finished!

index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nits, nice work!

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@jhugard
Copy link

jhugard commented Mar 22, 2024

Any thoughts on taking this as an opportunity to facilitate adding custom HTTP UPGRADE request headers?

For example, web clients currently must provide any auth token as a query parameter on the URL. This conflicts with security best practice, which recommend using an Authorization header to avoid accidental logging of security related info.

@ricea
Copy link
Collaborator Author

ricea commented Mar 25, 2024

Any thoughts on taking this as an opportunity to facilitate adding custom HTTP UPGRADE request headers?

I'm not going to add it in the initial PR.

I've considered adding it initially for WebSocketStream so we can experiment with a smaller and more adaptable user base. But we'd have to add it to the old WebSocket API too eventually. This is getting off-topic for this PR so please take future discussion to #16 (or don't -- that issue has already reached a length where most people won't read the whole thing).

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
WebIDL requires explicit defaults for optional arguments.

Co-authored-by: Philip Jägenstedt <[email protected]>
@dontcallmedom-bot
Copy link

This issue was mentioned in WEBRTCWG-2024-05-21 (Page 38)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs implementer interest Moving the issue forward requires implementers to express interest topic: websockets
Development

Successfully merging this pull request may close these issues.

None yet

9 participants