Add ability to close a channel to the transport interface

[iislucas]

At the moment there is no way to close a data channel belonging to the transport interface.

As well as leading to a memory leak, there are only 16 bits of channel identifiers for WebRTC transport, so transport will stop working after a bit too.

Suggested fix is to add something like a closeChannel method to the transport interface; and onCloseChannel for when the other side closes it.

[willscott]

Presumably, you mean that you want to signal that a 'tag' will no longer be used at the transport interface. I'm not sure that the semantics of the transport interface make that meaningful right now - there is no concept of starting up a channel - just an identifier of different flows going across the transport which we use to relax the in-order guarantee.

It seems like cleanup should instead be done within the web-rtc transport provider. In that case this is a dup of #13

[iislucas]

In that case, I think we've probably got a mis-match that is a bigger deal; the transport API, as is, is not what uProxy needs as it doesn't allow cleanup of data channels. This makes TCP connections not get closed properly, and also results in unreliable proxying after a while.

If we don't have a way to specify when a data channel has stopped being used, we also can't re-use the data channel. So I think this means that there isn't a way to implement TCP-to-RTC using the Transport API. As I need a solution fairly quickly, I'll move uProxy back to the peer-connection API which does work, and is, as far as I can see, pretty much the API that we actually want. Maybe we can take an evening this week to discuss some more.

@trevj (FYI)

[ryscheng]

Will we ever have anywhere even close to 2^16 active connections? If not, presumably there's a reasonable way for us to close out data channels automatically from the context of the transport provider. Why would it be important for UProxy to manually close out data channels? On Jun 15, 2014 9:31 PM, "Lucas Dixon" notifications@github.com wrote:

In that case, I think we've probably got a mis-match that is a bigger deal; the transport API, as is, is not what uProxy needs as it doesn't allow cleanup of data channels. This makes TCP connections not get closed properly, and also results in unreliable proxying after a while.

If we don't have a way to specify when a data channel has stopped being used, we also can't re-use the data channel. So I think this means that there isn't a way to implement TCP-to-RTC using the Transport API. As I need a solution fairly quickly, I'll move uProxy back to the peer-connection API which does work, and is, as far as I can see, pretty much the API that we actually want. Maybe we can take an evening this week to discuss some more.

@trevj https://github.com/trevj (FYI)

— Reply to this email directly or view it on GitHub https://github.com/freedomjs/freedom/issues/64#issuecomment-46140369.

[willscott]

I believe the worry is that they want to use that signal to know when the connection is closed explicitly. Why not have a 'signalling' data channel where explicit close notifications are sent? When the multiple streams are multiplexed across other transports you'll need those explicit messages anyway, and in practice they should be able to piggyback on the last payload datagram and not add much overhead. On Jun 15, 2014 9:51 PM, "Raymond Cheng" notifications@github.com wrote:

Will we ever have anywhere even close to 2^16 active connections? If not, presumably there's a reasonable way for us to close out data channels automatically from the context of the transport provider. Why would it be important for UProxy to manually close out data channels? On Jun 15, 2014 9:31 PM, "Lucas Dixon" notifications@github.com wrote:

In that case, I think we've probably got a mis-match that is a bigger deal; the transport API, as is, is not what uProxy needs as it doesn't allow cleanup of data channels. This makes TCP connections not get closed properly, and also results in unreliable proxying after a while.

If we don't have a way to specify when a data channel has stopped being used, we also can't re-use the data channel. So I think this means that there isn't a way to implement TCP-to-RTC using the Transport API. As I need a solution fairly quickly, I'll move uProxy back to the peer-connection API which does work, and is, as far as I can see, pretty much the API that we actually want. Maybe we can take an evening this week to discuss some more.

@trevj https://github.com/trevj (FYI)

— Reply to this email directly or view it on GitHub https://github.com/freedomjs/freedom/issues/64#issuecomment-46140369.

— Reply to this email directly or view it on GitHub https://github.com/freedomjs/freedom/issues/64#issuecomment-46140919.

[ryscheng]

I dont think that's the problem he's referring to. They already have a "control" data channel that they use for exactly this reason and it's straightforward how to get that signal.

On Sun, Jun 15, 2014 at 10:03 PM, Will notifications@github.com wrote:

I believe the worry is that they want to use that signal to know when the connection is closed explicitly. Why not have a 'signalling' data channel where explicit close notifications are sent? When the multiple streams are multiplexed across other transports you'll need those explicit messages anyway, and in practice they should be able to piggyback on the last payload datagram and not add much overhead. On Jun 15, 2014 9:51 PM, "Raymond Cheng" notifications@github.com wrote:

Will we ever have anywhere even close to 2^16 active connections? If not, presumably there's a reasonable way for us to close out data channels automatically from the context of the transport provider. Why would it be important for UProxy to manually close out data channels? On Jun 15, 2014 9:31 PM, "Lucas Dixon" notifications@github.com wrote:

In that case, I think we've probably got a mis-match that is a bigger deal; the transport API, as is, is not what uProxy needs as it doesn't allow cleanup of data channels. This makes TCP connections not get closed properly, and also results in unreliable proxying after a while.

If we don't have a way to specify when a data channel has stopped being used, we also can't re-use the data channel. So I think this means that there isn't a way to implement TCP-to-RTC using the Transport API. As I need a solution fairly quickly, I'll move uProxy back to the peer-connection API which does work, and is, as far as I can see, pretty much the API that we actually want. Maybe we can take an evening this week to discuss some more.

@trevj https://github.com/trevj (FYI)

— Reply to this email directly or view it on GitHub https://github.com/freedomjs/freedom/issues/64#issuecomment-46140369.

— Reply to this email directly or view it on GitHub https://github.com/freedomjs/freedom/issues/64#issuecomment-46140919.

— Reply to this email directly or view it on GitHub https://github.com/freedomjs/freedom/issues/64#issuecomment-46141282.

[willscott]

@iislucas - comment on this? I think explicit close messages would let you know to close the TCP connection. And that should be easier than re-implementing chunking over the core.peerconnection API. Is there something else I'm missing?

[iislucas]

I don't see a way, within the transport provider, to know that a data-channel should be closed. Outside the transport provider it's easy, but there is not way at the moment to tell transport close a data-channel. Hence the problem. :)

[ryscheng]

There's 2 questions I see.

1. Will there ever be over 2^16 active connections?
2. Does webrtc experience performance degradation when there are large numbers of unused data channels?

If the answers to both of these are no, then an LRU eviction policy will work. E.g. close the least recently used data channel when you reach your limit.

If the answer to some of these are yes, then the transport provider should pick some value n number of data channels for which to multiplex our connections over.

Either way, coding and depending against webrtc directly is a poor long-term design decision. If we are unhappy with the transport API as it is, we should discuss what problem we are solving and why it solving it should depend on a more complex interface as opposed to a more complex implementation. On Jun 16, 2014 6:18 AM, "Lucas Dixon" notifications@github.com wrote:

I don't see a way, within the transport provider, to know that a data-channel should be closed. Outside the transport provider it's easy, but there is not way at the moment to tell transport close a data-channel. Hence the problem. :)

— Reply to this email directly or view it on GitHub https://github.com/freedomjs/freedom/issues/64#issuecomment-46176451.

[iislucas]

Current options as I see them are:

1. LRU implementation + multiplexing over existing data-channels implemented within transport.
2. Multiplex over a fixed number of channels.
3. Use the current PC interface (without the buffered amount exposed, with chunking internalizes) as the P2P interface for transport.
4. Do something more like sockets where each transport implementation is exactly one data channel.

Option 1 provides non-deterministic behavior w.r.t. old channels. There's no way I'd do this. Option 2, which is what (1) falls back to anyway seems dumb given that we're already multiplexing in WebRTC and that each data channel is async from the others. Doing this will introduce subtle synchronous stuff again (and it's inefficiency); however in practice I could imagine it not being too terrible, except for having to have the code that implements it. Option 3 Is much simpler design than 1 and 2 but puts more burden on the user of the transport provider in that they need to close their created data channels - although this is expected and pretty much what every stream implementation does. For Option 4, it's not clear to me how to do it without first class module references. And this ends up being more or less option 3 as each one would have a close.

Option (1) and (2) can be implemented on top of (3). So for now looks to me like (3) is way better than the other choices both in terms of implementation time and design.

[ryscheng]

I'm still trying to get a better sense of what problem you're trying to solve though. Can you explain what you mean by "non-deterministic behavior w.r.t. old channels"?

On Mon, Jun 16, 2014 at 7:13 AM, Lucas Dixon notifications@github.com wrote:

Current options as I see them are:

1. LRU implementation + multiplexing over existing data-channels implemented within transport.
2. Multiplex over a fixed number of channels.
3. Use the current PC interface (without the buffered amount exposed, with chunking internalizes) as the P2P interface for transport.
4. Do something more like sockets where each transport implementation is exactly one data channel.

Option 1 provides non-deterministic behavior w.r.t. old channels. There's no way I'd do this. Option 2, which is what (1) falls back to anyway seems dumb given that we're already multiplexing in WebRTC and that each data channel is async from the others. Doing this will introduce subtle synchronous stuff again (and it's inefficiency); however in practice I could imagine it not being too terrible, except for having to have the code that implements it. Option 3 Is much simpler design than 1 and 2 but puts more burden on the user of the transport provider in that they need to close their created data channels - although this is expected and pretty much what every stream implementation does. For Option 4, it's not clear to me how to do it without first class module references. And this ends up being more or less option 3 as each one would have a close.

Option (1) and (2) can be implemented on top of (3). So for now looks to me like (3) is way better than the other choices both in terms of implementation time and design.

— Reply to this email directly or view it on GitHub https://github.com/freedomjs/freedom/issues/64#issuecomment-46183079.

[iislucas]

The limit on the number of channels in WebRTC/SCTP is a bit of a mess. 2^16 is the max theoretical. But I've seen problems with much lower numbers. For a while 9 was the limit.

The problem with LRU-grabage collection of data channels in my mind is 1. code complexity and 2. that the user of the transport provider does not know if send calls actually managed to send the message. Normally a user can tell shortly failure because the peer-connection gets closed. We might fix that by introducing error results for send, but now you have to deal with many kinds of failure. But this is getting pretty complex and unclear about when things will go wrong.

The problem with using a control channel for closing other channels is that it is async, so the close message may get to the other side before the last of the data. The result is that the other side may close before all data has arrived. Very uncool. Instead of using control, you can have an additional packet header to say whether the message is a control message or a data one. This is kind of redundant because WebRTC already has the PPID field of SCTP for this, which distinguishes between Blob, string, and arraybuffers. In addition SCTP already has close messages. But it's probably not a lot of data, just ugly and over-complex.

If the control channel was simply marking another channel for LRU-garbage collection, you'd be in a better situation, but this is still way over-complicating the problem, and still needs special failure cases for when a user tries to call send on something that has just been closed and for which there is no resources to open again. This naturally leads one to solution (2), which is better than (1), but I still prefer (3) by a way.

[willscott]

I do want to push back on the direct use of core.peerconnection, rather than transport. peerConnection is very unsuitable for (for instance) a socket transport - which has no notion of individual data channels, and does not need an array of STUN servers to function. peerConnection is clearly directly tied to webRTC, and not a general transport.

Beyond this, I think you're still going to need explicit closing messages to overlay the directional closing that TCP expects over the symetric WebRTC interpretation. Namely: The client is going to add it's fin packet to signal it is done sending to put the connection into a half-closed state while the server response comes in. It seems like that is also a thing we want the tunneled connection to do.

I feel like we can do that by having a message that says effectively 'tag closed after sending bytes'. Then the other side of socksRTC closes the TCP connection once the connection is drained. (and asynchronously transport.webRTC cleans up the data channel)

I don't follow why (2) is better than (1). Is it that you don't trust the transport to set the LRU size and want the application to control of how many channels are active?

[ryscheng]

Trying to keep my responses short, we can talk in person if we need higher bandwidth.

1. Code complexity - is better than interface complexity. It would be wrong to optimize for short-term code complexity at the cost of modularity and simple interfaces.
2. We cannot assume that calls to send will always succeed. Thus, we need error messages anyway.

In the same way it is wrong to tie our implementation to G+, I believe it to be wrong to tie our implementation to WebRTC. WebRTC is not fundamental to the design of uProxy (nor is it the ideal network stream abstraction by any means) and changes that depend on WebRTC now will be hard to change in the future. If you think we can do better, let's discuss that instead. Unless you truly believe WebRTC will be fundamental to any network transport we use in the future.

On Mon, Jun 16, 2014 at 8:11 AM, Will notifications@github.com wrote:

I do want to push back on the direct use of core.peerconnection, rather than transport. peerConnection is very unsuitable for (for instance) a socket transport - which has no notion of individual data channels, and does not need an array of STUN servers to function. peerConnection is clearly directly tied to webRTC, and not a general transport.

Beyond this, I think you're still going to need explicit closing messages to overlay the directional closing that TCP expects over the symetric WebRTC interpretation. Namely: The client is going to add it's fin packet to signal it is done sending to put the connection into a half-closed state while the server response comes in. It seems like that is also a thing we want the tunneled connection to do.

I feel like we can do that by having a message that says effectively 'tag closed after sending bytes'. Then the other side of socksRTC closes the TCP connection once the connection is drained. (and asynchronously transport.webRTC cleans up the data channel)

I don't follow why (2) is better than (1). Is it that you don't trust the transport to set the LRU size and want the application to control of how many channels are active?

— Reply to this email directly or view it on GitHub https://github.com/freedomjs/freedom/issues/64#issuecomment-46191005.

[iislucas]

1. w.r.t. complexity: I agree, but measurement of complexity should include understandability, behavior & usage complexity, not just number of lines in the file (e.g. consider SK combinator language for computation - small number of lines, but very very bad way to program).
2. agree.
3. w.r.t. goal of having general transport providers: we all agree.

Seems like we've motivated a deeper thinking about transport API; should it have channels? how should it close them? are channels async, what error messages do we get? Hopefully we can land at something close to what we have already. in v5? :)

[willscott]

-v0.5 Given that v0.5 is getting cut now, I'm going to say that changes to the API that come out of this will happen in the next breaking change. We can do explicit messages in the interim while we talk about it, but it seems important to to get v0.5 cut at some point, and I think that's going to happen tonight.

[iislucas]

fair enough :)

On Tue, Jun 17, 2014 at 12:05 AM, Will notifications@github.com wrote:

-v0.5 Given that v0.5 is getting cut now, I'm going to say that changes to the API that come out of this will happen in the next breaking change. We can do explicit messages in the interim while we talk about it, but it seems important to to get v0.5 cut at some point, and I think that's going to happen tonight.

— Reply to this email directly or view it on GitHub https://github.com/freedomjs/freedom/issues/64#issuecomment-46273893.

Lucas Dixon | Google Ideas

[trevj]

So, FWIW, I feel the notion of tags is already a leaky abstraction from WebRTC through to (the very generically named) transport API.

For example, I considered wrapping one of the obfuscating stuff's components into a transport provider -- specifically, the part that "takes over" the datachannel's ports. However, tags didn't really make any sense for that provider even though I considered it the most basic possible transport provider: it would just shuffle bits from one address to the other (maybe a "tag" could represent an "address"...but I didn't want to put that much thought into it).

I can tell you that lack of "type" was a big problem when I was refactoring the SOCKS server to use the transport API and it's why I had to create the "control channel" which is now giving us pause for consideration.

[iislucas]

Freedom 0.6 uses a lightweight webrtc provider; so this is no longer relevant. yay!