search

docker / libchan

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

Proposal: Use protocol defined in PROTOCOL.md #48

Open dmcgowan opened 9 years ago

dmcgowan commented 9 years ago

Moving protocol related discussion from #38 to here. This would follow the model Docker is adopting for large design changes, discussion is on the protocol and the PR is for code review and discussion related to whether it implements the proposal as defined.

Please read the PROTOCOL.md before commenting on this issue.

On going discussions

mcollina commented 9 years ago

(quoting @dmcgowan fully from #38)

The intention for channels is to not use the 'libchan-ref', which is mentioned in the PROTOCOL file as determining whether a stream is a channel or a bytestream. What is not mentioned is what would be used in its place. Since channels have a nested hierarchy, to me it makes sense to use the spdy stream ids which already have that hierarchy. In practice the hierarchy has not proven that useful yet and not currently supported in code (nothing is enforcing that a channel created by one channel cannot be sent on another, although must be the same session). However down the road I think it should be enforced and the hierarchy can be useful for debugging. As for possibly using the reference ids, the reference id does not have this hierarchy and we want to be able to use in the future to span multiple spdy sessions. Channels are more tied to their transport which makes using the stream id just less book keeping.

You are using it to discriminate between incoming requests, see https://github.com/dmcgowan/libchan/blob/interface_update/spdy/session.go#L99-L118 where the channels that does not have a parent are moved to the main receiver. This means that in node-land we need to have that parent id specified.

Does the node-spdy library you are using provide a higher level interface which abstracts the underlying streams away? I will take a look at your node implementation today and try and see what you guys are up against.

yes, there is no practical way for forcing a SPDY 'parent' stream through that interface. The API supported by node-spdy is HTTP-based on piggyback on node's http module, which means we don't have direct access to SPDY streams. The other solution is to ditch the HTTP-based thing (and use internal node-spdy stuff) or re-implement SPDY, but it's a major effort in any case.

The problem is only for nested channels sent from the client, as the server can do 'push', which works as expected.

dmcgowan commented 9 years ago

From the PROTOCOL.md under Top Level Channels (we need section numbers!)

When an endpoint receives a new inbound SPDY stream, and the initial headers DO NOT include the key libchan-ref, it MUST queue a new Receiver channel to pass to the application.

To do it the way you are suggesting involved changing the spec, which is fine. Since you are proposing not using the stream id, the question is would the byte stream reference id be appropriate, or require another identifier which is channel specific. If the reference id is going to be used, then channels will have to include a different header to specify it is a channel and whether it has a parent channel.

mcollina commented 9 years ago

When an endpoint receives a new inbound SPDY stream, and the initial headers DO NOT include the key libchan-ref, it MUST queue a new Receiver channel to pass to the application.

I am proposing to use 'libchan-ref' for everything, enforcing that sentence as it is (or as I understand it). In fact, what is missing right now in that sentence is "and it has no parent SPDY stream".

dmcgowan commented 9 years ago

@mcollina implemented the above with an additional header for channels called 'libchan-parent-ref' https://github.com/dmcgowan/libchan/commit/de99fa22bba323fd16b6e6e2039a7f0e8b29aa16

Also have a rebased rexec which uses this https://github.com/dmcgowan/libchan/commit/3d21143c02e2510330e161585df34795f17a92be We can also come up with a different example that we can use to test integration.

mcollina commented 9 years ago

Thanks Derek. I'm traveling the next couple of days, I'll try it on monday.

mcollina commented 9 years ago

@dmcgowan from time to time your rexec server does not return anything on the embedded streams, both in the go and node versions.

Anyway, it works, we are getting closer! Thanks for your awesome support!

dmcgowan commented 9 years ago

I would say open an issue but perhaps after the rexec is merged in. Are you able to reproduce it just after making repeated calls to longer-lived server?

mcollina commented 9 years ago

Here is the feeling: the server kicks out the status message before the copying of stdout is completed. I have this very same issue on the node counterparts, so it's kind of ok anyway.