docker / libchan

Like Go channels over the network
Apache License 2.0
2.47k stars 142 forks source link

Update listener to use stream session #27

Closed dmcgowan closed 10 years ago

dmcgowan commented 10 years ago

Listener is currently broken when multiple connections are created, using sessions allows proper separation of each connection. A sync condition is added around stream chans to avoid race conditions between setting stream chans and reading them. The original behavior used the global stream chan when the stream was not found, however that led to synchronization issues when sender/receivers sometimes got back different channels.

smarterclayton commented 10 years ago

Can you make NewStreamSession public? That's what a websocket transport needs to expose (one http2 connection, server flag = true)

dmcgowan commented 10 years ago

The behavior of NewStreamSession has not changed, only change is the listener now uses the private function to create a server session. Could the websocket transport use the listener session? The purpose of not directly exposing the ability to create a server session is then sessions could easily be created without going through the authenticator function.

smarterclayton commented 10 years ago

For websockets there is only a single connection and it's always the server. The authenticator is also handled partially by websockets (via existing http auth or via ws auth), although we can manually invoke the authenticator.

dmcgowan commented 10 years ago

I'll take a look at the ws stuff you are porting. I think it is reasonable to have a server session apart from listen session, but that can be separated into a different change. I would like to not change the interface of NewStreamSession, but there can be another function to create a server session.

shykes commented 10 years ago

@dmcgowan you are the sole maintainer for this part, so technically you can merge it on your own.

A few notes on "single maintainer" etiquette:

shykes commented 10 years ago

This didn't build when I ran the tests.

It looks like a version mismatch in spdystream. I will update on HEAD and test again, but this is probably a good time to introduce Godep?

dmcgowan commented 10 years ago

I think Godep will be useful, although I always find Godep awkward for libraries.

shykes commented 10 years ago

Confirming that all tests pass when pulling HEAD of spdystream

smarterclayton commented 10 years ago

I'll remerge WS on top of this if it's ready.

shykes commented 10 years ago

LGTM