WebRTC Data Channel over WHATWG Streams

Question

There's been talk about implementing WebRTC Data Channel over WHATWG Streams for a while

• https://github.com/w3c/webrtc-pc/issues/1732
• https://github.com/w3c/webrtc-nv-use-cases/issues/44
• https://github.com/w3c/webrtc-rtptransport/issues/8
• https://www.w3.org/2023/12/05-webrtc-minutes.html#t14

AFAICT nobody has written out the draft specification or implemented WebRTC Data Channels using WHATWG Streams, yet.

Here's the start of my implementation.

Do I want feedback about any or all facets of the code?

any aspect of the code posted is fair game for feedback and criticism.

var channel = local.createDataChannel("transfer", {
 negotiated: true,
 ordered: true,
 id: 0,
 binaryType: "arraybuffer",
 protocol: "udp",
});
var readableController;
var writableController;
var { resolve, promise: dataChannelStream } = Promise.withResolvers();
// var writer = writable.getWriter();
channel.onopen = async (e) => {
 console.log(e.type, e.target);
 var readable = new ReadableStream({
 start(_) {
 return readableController = _;
 },
 cancel(reason) {
 console.log(reason);
 },
 });
 var writable = new WritableStream({
 start(_) {
 return writableController = _;
 },
 write(v) {
 console.log(v);
 },
 close() {
 console.log("writable close");
 channel.close();
 },
 abort(reason) {
 console.log(reason);
 },
 });
 readable.pipeTo(
 new WritableStream({
 write(v) {
 channel.send(v);
 },
 }),
 ).catch(() => channel.close());
 resolve({ readable, writable });
};
channel.onclose = async (e) => {
 console.log(e.type, e.target);
 await Promise.allSettled([readable.cancel(), writable.close()])
 .then(() => console.log("streams closed")).catch(console.log);
};
channel.onclosing = async (e) => {
 console.log(e.type);
};
channel.onbufferedamountlow = (e) => {
 console.log(e.type, channel.bufferedAmount);
};
channel.onerror = async (e) => {
 console.log(e.type, e.target);
 await Promise.allSettled([readable.cancel(), writable.close()])
 .then(() => console.log("streams closed")).catch(console.log);
};
channel.onmessage = (e) => {
 readableController.enqueue(e.data);
};
var offer = await local.createOffer({
 voiceActivityDetection: false,
});
local.setLocalDescription(offer);
var { readable, writable } = await dataChannelStream;
await new Response("test").body.pipeTo(writable, { preventClose: 1 });

Answer 1

Using var seems a bit legacy to me, why not use let or const as appropriate?

The underscore _ is often used to hold something one is not interested in, that can be discarded. Here, you use it in the start methods of the streams, assign it to the respective controllers. So why not give it a proper name?

• guest271314: Is that it? I use var because I use the code in Snippets in DevTools, so I can re-run the code without getting a syntax error for redeclaring a variable defined using let or const. I could give the default controller a name. Won't hurt nothing.
• guest271314: Isn't the logic wrong? Shouldn't the code be using channel.send(v) in write() of WritableStream() constructor, and using console.log(v) in the WritableStream chained to readable.pipeTo()?
• J_H: @guest271314, having a nice edit - debug cycle is important, certainly. But you can just bury things in a throwaway { } block. Then you can repeat the let to your heart's content, as you're creating a variable of same name within a new scope, e.g. { let x = 42 } and then change your mind to use a different number.
• guest271314: @J_H The const and let thing is superficial. If you are going to review you should probably catch that as written in OP piping to writable doesn't go over WebRTC.

Answer 2

spec

The Review Context didn't mention the version of the WebRTC implementation being targeted, nor which WHATWG spec we claim to conform to.

It is perfectly fine for an initial prototype to implement just a subset of the spec; Rome wasn't built in a day. But there should be some text that clearly identifies which parts we're compliant with and which parts await future work.

choosing identifiers

channel.onopen = async (e) => {

I'm sure anyone skilled in the art would immediately read "e" as "event". But I wouldn't mind seeing that word appear just once in a comment, to remind me. Alternatively, consider putting a documentation URL pointer in the source code, which describes the high level concepts involved and details like WebRTC's 64 KiB chunk limits which WHATWG streams don't suffer from (the original motivation for the project).

The point is that e is a formal parameter in an exported Public API, so the documentation burden is higher than it would be for just a local temp var.

Lines like start(_) { are quite mystifying. I can't imagine why you don't want to give the caller a hint about what goes there. Suppose we fail within that block -- would you really want to read a stacktrace that omits meaningful identifiers? During a telephone support call would you really want to keep reciting "underscore" instead of a proper variable name?

Also, the commented assignment to writer was fine during initial debugging, but please delete such commented code by the time the codebase becomes mature enough to advance through a review stage.

debugs

 console.log(e.type, e.target);

This, and similar log() messages, seem

• rather chatty, and
• less descriptive than a maintenance engineer might want them to be

Consider adopting an approach that lets us distinguish between .info() and .debug() severity, better identifies the call site and what's happening, and supports suppressing "chatty" messages during routine production runs.

test suite

The OP includes no automated tests. Please add at least two tests, to motivate why this project is helpful to app developers:

1. Send a 70 KiB file via native WebRTC
2. Send same file using this library

Hopefully there's less code needed to put together that 2nd test, demonstrating the library's expressive power to its prospective users.

A more ambitious testing goal would be to verify that backpressure works as expected, slowing down the sender and (measurably) preventing buffer bloat.

• guest271314: You missed that as written the pipeTo(writable) doesn't go over WebRTC, too. I fixed that. It works as expected now. I'm on Chromium 140. WHATWG Streams. It's a specification itself.
• guest271314: Re "test suite". Here's the working code github.com/guest271314/sockets/blob/fetch-webrtc/README.md, slightly rewritten to fix what both answers here missed. That this await new Response("test").body.pipeTo(writable, { preventClose: 1 }); is only going to the console.log here var writable = new WritableStream({start(_) {return writableController = _;},write(v) {console.log(v);}, in the code at OP, is not going over a WebRTC Data Channel. Neither answer here caught that. Both are preoccupied with var, which is inconsequential to the requirement. Clearly neither answerer tested.