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

No way to observe DataChannel-only transport events in initial negotiation #2899

Open
jan-ivar opened this issue Aug 28, 2023 · 13 comments
Open

Comments

@jan-ivar
Copy link
Member

jan-ivar commented Aug 28, 2023

As mentioned in #2898, whereas transceiver.transports are surfaced in sLD, pc.sctp isn't surfaced until the answer.

Inconsistency aside, this renders the following event handlers kinda useless on initial negotiation:

  1. pc.sctp.onstatechange
  2. pc.sctp.transport.onstatechange
  3. pc.sctp.transport.iceTransport.ongatheringstatechange
  4. pc.sctp.transport.iceTransport.onselectedcandidatepairchange

...because it's not possible to register handlers early enough to catch events reliably. It also means they work differently on initial vs. subsequent negotiations, which isn't great.

There's no other way to get dtls or ice-transport-object events for DataChannel-only connections.

Workarounds

  1. (For some states) use pc.oniceconnectionstatechange and pc.onicegatheringstatechange instead, or
  2. use "max-bundle" and add a dummy audio transceiver to surface the DTLS and ICE transport earlier

This seems broken. We should probably surface the sctp transport in sLD (and handle it in rollback) like the others.

@jan-ivar
Copy link
Member Author

This change would require either making maxMessageSize nullable or just initializing it to 0.

@lgrahl
Copy link
Contributor

lgrahl commented Aug 28, 2023

The suggestion would be a big downgrade to me, let me explain:

I consider it good API design that RTCSctpTransport is not available prior to being negotiated because it prevents a bunch of irrelevant default values in case it is not negotiated / not used. This is great for static code analysis. If I have unwrapped the sctp property, then I

a) know that SCTP has been negotiated, and
b) consequently all properties inside of it contain meaningful values.

If we were to switch this around and make the sctp property itself non-nullable, but properties like maxMessageSize inside it nullable, then suddenly maxMessageSize would be of type number | null in TypeScript. That would be super annoying because if SCTP has been negotiated, I know that it can't be null, yet would still have to handle null every time I access this and any potential other property.

With the benefits of the status quo logic in mind, if we have some other property where the information is relevant prior to negotiation, then it does not belong on RTCSctpTransport but rather on RTCPeerConnection. However, as of now I don't think such a property exists. Not even the SCTP state because that state is irrelevant prior to negotiation. After negotiation we initialise it with 'connecting'. What further initial information is there to gain?
The transport property on the other hand just seems to be a convenience thing to me.

Edit: We also discussed a while ago that SCTP is different to the other transports. It does not allow for any kind of renegotiation by use of SDP, so this design remains plausible for the lifetime of the peer connection. (See #2839 and some other discussion I couldn't find where we discussed what should happen in case max-message-size changes between negotiations).

@jan-ivar
Copy link
Member Author

jan-ivar commented Aug 29, 2023

... it prevents a bunch of irrelevant default values in case it is not negotiated / not used

I don't see a "bunch", only maxMessageSize as mentioned:

interface RTCSctpTransport : EventTarget {
  readonly attribute RTCDtlsTransport transport;
  readonly attribute RTCSctpTransportState state;
  readonly attribute unrestricted double maxMessageSize;
  readonly attribute unsigned short? maxChannels;
  attribute EventHandler onstatechange;
};

maxChannels is already nullable, and "will be null until the SCTP transport goes into the "connected" state."
state starts out as "connecting" which still works as initial state I think once exposed in sLD.

If we were to switch this around and make the sctp property itself non-nullable,

No, sctp would remain nullable, like all transports to be surfaced in sLD.

but properties like maxMessageSize inside it nullable, then suddenly maxMessageSize would be of type number | null in TypeScript.

Since this is already true for maxChannels, this doesn't seem like a big win.

Edit: We also discussed a while ago that SCTP is different to the other transports. It does not allow for any kind of renegotiation by use of SDP, so this design remains plausible for the lifetime of the peer connection.

I believe SCTP can be added in renegotiation, just not removed? In any case, what difference does that make toward solving the OP problem?

@jan-ivar jan-ivar changed the title No way to register for transport events in DataChannel-only initial negotiation No way to observe DataChannel-only transport events in initial negotiation Sep 7, 2023
@lgrahl
Copy link
Contributor

lgrahl commented Sep 9, 2023

Sure, the design does not align well with my expectations in the case of maxChannels and any other potential property bound to anything beyond the negotiated state, because those are only available in the next state and therefore have to be nullable.

If I were to spec this API freely, I would have preferred to assign each state its own set of properties and make it more friendly to static analysis as a side effect, e.g. something like the following (TS syntax):

type SctpTransportState =
  | {
      name: 'staged';
    }
  | {
      name: 'negotiated';
      maxMessageSize: number;
    }
  | {
      name: 'connected';
      maxMessageSize: number;
      maxChannels: number;
    };

interface SctpTransport {
  state: SctpTransportState;
  onstatechange: ...;
}

Therefore, I guess my view of the design does not align with your/the spec authors view of the design and the result just aligned by accident to some degree.

@jan-ivar
Copy link
Member Author

Thanks for the discriminated union example. That's something to consider in future APIs for sure.

Any change this late probably has to be surgical to minimize breakage.

Every design comes with tradeoffs. E.g. if we were to add a "new" state distinct from "connecting", it might help, but A) be somewhat redundant with signalingState, B) probably cause breakage, and C) still not solve what to put in the "closed" state:

type RTCSctpTransport =
  | null | {
    state: "new";
    transport: object;
    onstatechange: object;
  } | {
    state: "connecting";
    transport: object;
    maxMessageSize: number;
    onstatechange: object;
  } | {
    state: "connected";
    transport: object;
    maxMessageSize: number;
    maxChannels: number;
    onstatechange: object;
  } | {
    state: "closed";
    transport: object;
    maxMessageSize: number; // present or not?
    maxChannels: number; // present or not?
    onstatechange: object;
  };

interface RTCPeerConnection {
  sctp: RTCSctpTransport;
}

Whether maxMessageSize, maxChannels, or both are present in "closed" would depend on when it closed.

@alvestrand
Copy link
Contributor

I wouldn't mind having a way to get the transport that worked for all cases independent of whether there was SCTP or not; a bunch of tests are doing sctp.transport or senders[0].transport at random. The sticker here is really what to return for non-bundled connections.

@steely-glint
Copy link

In the TPAC meeting Henrik mentioned a variant of proposal B - adding a member to the pc.oniceconnectionstatechange data that is the iceTransport that it relates to - this seems cleaner and simpler than adding an (array of) iceTransports properties to the Peer connection.

I don't like surfacing a (potentially ghost) sctpTransport who's values are wrong or non-existent just to get an iceTransport.

@jan-ivar
Copy link
Member Author

... a way to get the transport that worked for all cases independent of whether there was SCTP ...
The sticker here is really what to return for non-bundled connections.

Agree this might've worked if "max-bundle" was all we needed to support. But the current API surface shape also supports "balanced" and "max-compat".

adding a member to the pc.oniceconnectionstatechange data that is the iceTransport that it relates to

This has the same problem Harald raises: what to return for non-bundled connections?

I also thought we introduced the ORTC-like transport objects to get away from relying on the aggregated pc states, so building on the aggregated state feels like the wrong direction.

I don't like surfacing a (potentially ghost) sctpTransport who's values are wrong or non-existent just to get an iceTransport.

Aren't transceivers also ghosts by this definition? I don't think its values are more wrong than those of DtlsTransport: they reflect what the local offer is about to negotiate.

@alvestrand
Copy link
Contributor

I was imagining a getter that returns all the transports currently active for the PeerConnection. When using max-bundle, there would be only one; when there are more than one, apps that care will have to find some way to sort out which is which. (Comparing the transport objects to those on the transceivers should do it).

I don't know how you define "ghost" when you say that "transceivers are ghosts" - the scenario for the sctpTransport is that when the responder rejects the "m=application" media section in the remote answer, no sctpTransport underlying object will ever have been created. So the sctpTransport object that would be visible after createOffer will never correspond to a real sctpTransport - unlike rejected transceivers, which "just" transition to state "stopped" (if I remember rightly).

That said, as I said in the meeting, I won't stand in the way of resolving this issue by mandating the creation of the ghost.
(Does the ghost go away when it's known that it will never be created? If not, what's its state? Details, details.....)

@jan-ivar
Copy link
Member Author

when the responder rejects the "m=application" media section in the remote answer ...

Not to branch out too many new topics, but how would an app do that (in the API?) pc.sctp.stop()? 🤔🤷🏼‍♂️

So the sctpTransport object that would be visible after createOffer will never correspond to a real sctpTransport -

This would match how the other transports work.

unlike rejected transceivers, which "just" transition to state "stopped" (if I remember rightly).

Rejected transceivers eventually disappear from getTransceivers() — we just kludged it to require an offer from the remote side first to avoid apps accidentally nuking the offerer-tagged BUNDLE transport.

(Does the ghost go away when it's known that it will never be created? If not, what's its state? Details, details.....)

Same as with the other transports. They stop being referenced by the pc, so the app can no longer find them. But of course JS can hold any object alive, so its states are still observable and need to make sense. It's state would be closed I imagine.

@alvestrand
Copy link
Contributor

when the responder rejects the "m=application" media section in the remote answer ...

Not to branch out too many new topics, but how would an app do that (in the API?) pc.sctp.stop()? 🤔🤷🏼‍♂️

by not being a WebRTC implementation. We in fact had exactly this issue interoperating with another company's server (I suspect it was a SIP server).

So the sctpTransport object that would be visible after createOffer will never correspond to a real sctpTransport -

This would match how the other transports work.

No - the ICETransport underlying object is created on SLD, I think the DTLSTransport object is too. Then they are destroyed again when they turn out not to be needed. But that is really an implementation detail - we could have created ghosts.

unlike rejected transceivers, which "just" transition to state "stopped" (if I remember rightly).

Rejected transceivers eventually disappear from getTransceivers() — we just kludged it to require an offer from the remote side first to avoid apps accidentally nuking the offerer-tagged BUNDLE transport.

But if you hang on to a reference to them, they don't disappear at all....

(Does the ghost go away when it's known that it will never be created? If not, what's its state? Details, details.....)

Same as with the other transports. They stop being referenced by the pc, so the app can no longer find them. But of course JS can hold any object alive, so its states are still observable and need to make sense. It's state would be closed I imagine.

That makes sense.

@jan-ivar
Copy link
Member Author

@steely-glint introduced the term "ghost", which I interpreted to mean an object that, if rejected or BUNDLED away by the other side, only exists during negotiation (or in the case of transceivers, until a negotiation initiated by the other side).

The proposal here is to align the sctpTransport's lifetime with that of its iceTransport, which acts as @alvestrand describes, which I find quite ghostly, but regardless.

I wrote this fiddle to reject m=application and found quite inconsistent behavior in browsers (bugs maybe?) But all of the browsers seem to fire ice candidates on initial negotiation only, so we might only need to expose this pc.sctp ghost once, and not on every renegotiation which is a relief.

Datachannel rejection seems like an edge-case. But how are apps to learn of it? Might early exposure help here? E.g.

pc.createDataChannel("");
await pc.setLocalDescription();
pc.sctp.onstatechange = () => pc.sctp.state == "closed" && console.log("m=application rejected!");
// continue negotiation

I find support for this in the description of the "closed" state: "the SCTP association has been closed intentionally, such as by closing the peer connection or applying a remote description that rejects data or changes the SCTP port."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants