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.

Sign up for GitHub

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

Properly handle data channel opening failures #2936

Merged
merged 4 commits into from
Oct 27, 2022
Merged

Properly handle data channel opening failures #2936

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4442c15
Properly handle data channel opening failures
tomaka Oct 27, 2022
e2edcf2
Handle handshakeDataChannel closing before reported
tomaka Oct 27, 2022
0ad36f7
CHANGELOG
tomaka Oct 27, 2022
488116f
Use an asynchronous function instead of setTimeout
tomaka Oct 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions bin/light-base/src/platform.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,11 @@ pub trait Platform: Send + 'static {
/// Queues the opening of an additional outbound substream.
///
/// The substream, once opened, must be yielded by [`Platform::next_substream`].
///
/// > **Note**: No mechanism exists in this API to handle the situation where a substream fails
/// > to open, as this is not supposed to happen. If you need to handle such a
/// > situation, either try again opening a substream again or reset the entire
/// > connection.
fn open_out_substream(connection: &mut Self::Connection);

/// Waits until a new incoming substream arrives on the connection.
Expand Down
203 changes: 133 additions & 70 deletions bin/wasm-node/javascript/src/index-browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,38 +148,126 @@ export function start(options?: ClientOptions): Client {

// TODO: detect localhost for Firefox? https://bugzilla.mozilla.org/show_bug.cgi?id=1659672

let pc: RTCPeerConnection | null = null;
// Note that `pc` can be the connection, but also null or undefined.
// `undefined` means "certificate generation in progress", while `null` means "opening must
// be cancelled".
// While it would be better to use for example a string instead of `null`, using `null` lets
// us use the `!` operator more easily and leads to more readable code.
let pc: RTCPeerConnection | null | undefined = undefined;
// Contains the data channels that are open and have been reported to smoldot.
const dataChannels = new Map<number, RTCDataChannel>();
// TODO: this system is a complete hack
let isFirstSubstream = true;
// The opening of the connection is asynchronous. If smoldot calls `close` in the meanwhile,
// this variable is set to `true`, and we interrupt the opening.
let cancelOpening = false;
// For various reasons explained below, we open a data channel in advance without reporting it
// to smoldot. This data channel is stored in this variable. Once it is reported to smoldot,
// it is inserted in `dataChannels`.
let handshakeDataChannel: RTCDataChannel | undefined;
// Multihash-encoded DTLS certificate of the local node. Unknown as long as it hasn't been
// generated.
// TODO: could be merged with `pc` in one variable, and maybe even the other fields as well
let localTlsCertificateMultihash: Uint8Array | undefined;

// Kills all the JavaScript objects (the connection and all its substreams), ensuring that no
// callback will be called again. Doesn't report anything to smoldot, as this should be done
// by the caller.
const killAllJs = () => {
// The `RTCPeerConnection` is created pretty quickly. It is however still possible for
// smoldot to cancel the opening, in which case `pc` will still be undefined.
if (!pc) {
console.assert(dataChannels.size === 0 && !handshakeDataChannel, "substreams exist while pc is undef")
pc = null;
return
}

pc!.onconnectionstatechange = null;
pc!.onnegotiationneeded = null;
pc!.ondatachannel = null;

for (const channel of Array.from(dataChannels.values())) {
channel.onopen = null;
channel.onerror = null;
channel.onclose = null;
channel.onmessage = null;
}
dataChannels.clear();
if (handshakeDataChannel) {
handshakeDataChannel.onopen = null;
handshakeDataChannel.onerror = null;
handshakeDataChannel.onclose = null;
handshakeDataChannel.onmessage = null;
}
handshakeDataChannel = undefined;

pc!.close(); // Not necessarily necessary, but it doesn't hurt to do so.
};

// Function that configures a newly-opened channel and adds it to the map. Used for both
// inbound and outbound substreams.
const addChannel = (dataChannel: RTCDataChannel, direction: 'inbound' | 'outbound') => {
const addChannel = (dataChannel: RTCDataChannel, direction: 'first-outbound' | 'inbound' | 'outbound') => {
const dataChannelId = dataChannel.id!;
dataChannel.binaryType = 'arraybuffer';

let isOpen = false;

dataChannel.onopen = () => {
console.assert(!isOpen, "substream opened twice")
isOpen = true;

if (direction === 'first-outbound') {
console.assert(dataChannels.size === 0, "dataChannels not empty when opening");
console.assert(handshakeDataChannel === dataChannel, "handshake substream mismatch");
config.onOpen({
type: 'multi-stream',
handshake: 'webrtc',
// `addChannel` can never be called before the local certificate is generated, so this
// value is always defined.
localTlsCertificateMultihash: localTlsCertificateMultihash!,
remoteTlsCertificateMultihash: remoteCertMultihash
});
} else {
console.assert(direction !== 'outbound' || !handshakeDataChannel, "handshakeDataChannel still defined");
config.onStreamOpened(dataChannelId, direction);
}
};

dataChannel.onerror = (_error) => {
config.onStreamReset(dataChannelId);
};

dataChannel.onclose = () => {
dataChannel.onerror = dataChannel.onclose = (error) => {
// A couple of different things could be happening here.
if (handshakeDataChannel === dataChannel && !isOpen) {
// The handshake data channel that we have opened ahead of time failed to open. As this
// happens before we have reported the WebRTC connection as a whole as being open, we
// need to report that the connection has failed to open.
killAllJs();
config.onConnectionReset("handshake data channel failed to open" + error ? " " + error.toString() : "");
} else if (handshakeDataChannel === dataChannel) {
// The handshake data channel has been closed before we reported it to smoldot. This
// isn't really a problem. We just update the state and continue running. If smoldot
// requests a substream, another one will be opened. It could be a valid implementation
// to also just kill the entire connection, however doing so is a bit too intrusive and
// punches through abstraction layers.
handshakeDataChannel.onopen = null;
handshakeDataChannel.onerror = null;
handshakeDataChannel.onclose = null;
handshakeDataChannel.onmessage = null;
handshakeDataChannel = undefined;
} else if (!isOpen) {
// Substream wasn't opened yet and thus has failed to open. The API has no mechanism to
// report substream openings failures. We could try opening it again, but given that
// it's unlikely to succeed, we simply opt to kill the entire connection.
killAllJs();
config.onConnectionReset("data channel failed to open" + error ? " " + error.toString() : "");
} else {
// Substream was open and is now closed. Normal situation.
config.onStreamReset(dataChannelId);
}
};

dataChannel.onmessage = (m) => {
// The `data` field is an `ArrayBuffer`.
config.onMessage(new Uint8Array(m.data), dataChannelId);
// The `data` field is an `ArrayBuffer`.
config.onMessage(new Uint8Array(m.data), dataChannelId);
}

dataChannels.set(dataChannelId, dataChannel);
if (direction !== 'first-outbound')
dataChannels.set(dataChannelId, dataChannel);
else
handshakeDataChannel = dataChannel
}

// It is possible for the browser to use multiple different certificates.
Expand All @@ -188,7 +276,7 @@ export function start(options?: ClientOptions): Client {
// According to <https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-generatecertificate>,
// browsers are guaranteed to support `{ name: "ECDSA", namedCurve: "P-256" }`.
RTCPeerConnection.generateCertificate({ name: "ECDSA", namedCurve: "P-256", hash: "SHA-256" } as EcKeyGenParams).then(async (localCertificate) => {
if (cancelOpening)
if (pc === null)
return;

// Create a new WebRTC connection.
Expand Down Expand Up @@ -229,7 +317,7 @@ export function start(options?: ClientOptions): Client {
config.onConnectionReset('Failed to obtain the browser certificate fingerprint');
return;
}
const localTlsCertificateMultihash = new Uint8Array(34);
localTlsCertificateMultihash = new Uint8Array(34);
localTlsCertificateMultihash.set([0x12, 32], 0);
localTlsCertificateMultihash.set(localTlsCertificateHex!.split(':').map((s) => parseInt(s, 16)), 2);

Expand All @@ -240,21 +328,8 @@ export function start(options?: ClientOptions): Client {
// open.
pc.onconnectionstatechange = (_event) => {
if (pc!.connectionState == "closed" || pc!.connectionState == "disconnected" || pc!.connectionState == "failed") {
killAllJs();
config.onConnectionReset("WebRTC state transitioned to " + pc!.connectionState);

pc!.onconnectionstatechange = null;
pc!.onnegotiationneeded = null;
pc!.ondatachannel = null;

for (const channel of Array.from(dataChannels.values())) {
channel.onopen = null;
channel.onerror = null;
channel.onclose = null;
channel.onmessage = null;
}

pc!.close(); // Not necessarily necessary, but it doesn't hurt to do so.
dataChannels.clear();
}
};

Expand Down Expand Up @@ -342,49 +417,27 @@ export function start(options?: ClientOptions): Client {
};

pc.ondatachannel = ({ channel }) => {
// TODO: is the substream maybe already open? according to the Internet it seems that no but it's unclear
addChannel(channel, 'inbound')
};

// Creating a `RTCPeerConnection` doesn't actually do anything before a channel is created.
// The connection is therefore immediately reported as opened to smoldot so that it starts
// opening substreams.
// One concern might be that smoldot will think that the remote is reachable at this address
// (because we report the connection as being open) even when it might not be the case.
// However, WebRTC has a handshake to perform, and smoldot will only consider a connection
// as "actually open" once the handshake has finished.
config.onOpen({
type: 'multi-stream',
handshake: 'webrtc',
localTlsCertificateMultihash,
remoteTlsCertificateMultihash: remoteCertMultihash
});
// Creating a `RTCPeerConnection` doesn't actually do anything before `createDataChannel` is
// called. Smoldot's API, however, requires you to treat entire connections as open or
// closed. We know, according to the libp2p WebRTC specification, that every connection
// always starts with a substream where a handshake is performed. After we've reported that
// the connection is open, smoldot will open a substream in order to perform the handshake.
// Instead of following this API, we open this substream in advance, and will notify smoldot
// that the connection is open when the substream is open.
// Note that the label passed to `createDataChannel` is required to be empty as per the
// libp2p WebRTC specification.
addChannel(pc!.createDataChannel("", { id: 0, negotiated: true }), 'first-outbound')
});

return {
reset: (streamId: number | undefined): void => {
// If `streamId` is undefined, then the whole connection must be destroyed.
if (streamId === undefined) {
// The `RTCPeerConnection` is created at the same time as we report the connection as
// being open. It is however possible for smoldot to cancel the opening, in which case
// `pc` will still be undefined.
if (!pc) {
cancelOpening = true;
return;
}

pc.onconnectionstatechange = null;
pc.onnegotiationneeded = null;
pc.ondatachannel = null;

for (const channel of Array.from(dataChannels.values())) {
channel.onopen = null;
channel.onerror = null;
channel.onclose = null;
channel.onmessage = null;
}

pc.close();
dataChannels.clear();
killAllJs();

} else {
const channel = dataChannels.get(streamId)!;
Expand All @@ -404,12 +457,22 @@ export function start(options?: ClientOptions): Client {
openOutSubstream: () => {
// `openOutSubstream` can only be called after we have called `config.onOpen`, therefore
// `pc` is guaranteed to be non-null.
// Note that the label passed to `createDataChannel` is required to be empty as per the
// libp2p WebRTC specification.
if (isFirstSubstream) {
isFirstSubstream = false;
addChannel(pc!.createDataChannel("", { id: 0, negotiated: true }), 'outbound')
// As explained above, we open a data channel ahead of time. If this data channel is still
// there, we report it.
if (handshakeDataChannel) {
// Use `setTimeout` because calling callbacks within callbacks is error-prone.
setTimeout(() => {
// We need to check again if `handshakeDataChannel` is still defined, as the
// connection might have been closed.
if (handshakeDataChannel) {
config.onStreamOpened(handshakeDataChannel.id!, 'outbound')
dataChannels.set(handshakeDataChannel.id!, handshakeDataChannel)
handshakeDataChannel = undefined
}
}, 1)
} else {
// Note that the label passed to `createDataChannel` is required to be empty as per the
// libp2p WebRTC specification.
addChannel(pc!.createDataChannel(""), 'outbound')
}
}
Expand Down
4 changes: 4 additions & 0 deletions bin/wasm-node/javascript/src/instance/bindings-smoldot-light.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,10 @@ export interface Config {
* connections of type "multi-stream".
*
* The `onStreamOpened` callback must later be called with an outbound direction.
*
* Note that no mechanism exists in this API to handle the situation where a substream fails
* to open, as this is not supposed to happen. If you need to handle such a situation, either
* try again opening a substream again or reset the entire connection.
*/
openOutSubstream(): void;
}
Expand Down
5 changes: 5 additions & 0 deletions bin/wasm-node/rust/src/bindings.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,11 @@ extern "C" {
///
/// This function will only be called for multi-stream connections. The connection must
/// currently be in the `Open` state. See the documentation of [`connection_new`] for details.
///
/// > **Note**: No mechanism exists in this API to handle the situation where a substream fails
/// > to open, as this is not supposed to happen. If you need to handle such a
/// > situation, either try again opening a substream again or reset the entire
/// > connection.
pub fn connection_stream_open(connection_id: u32);

/// Abruptly closes an existing substream of a multi-stream connection. The substream must
Expand Down