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

WIP: New mux implementation #4

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

WIP: New mux implementation #4

wants to merge 3 commits into from

Conversation

pietgeursen
Copy link
Member

Making a PR so we can track it!

@sbillig sbillig changed the title New mux implementation; still a bit rough WIP: New mux implementation Mar 16, 2020
@sbillig sbillig self-requested a review March 16, 2020 21:14
@sbillig sbillig marked this pull request as ready for review March 16, 2020 21:14
@sbillig
Copy link
Member

sbillig commented Mar 16, 2020

@pietgeursen

Regarding closing child streams... (Thinking out loud, but comments are welcome.)

I don't know how other implementations handle the 'end' packet for duplex (child) streams. Does 'end' mean that I'm done sending packets on the stream, but I'll continue waiting for your packets until you say you're done? Or does it mean that I'm done with the stream altogether, and you shouldn't bother sending any more packets because I'm just going to ignore them? We need to know when to close and remove the incoming side of the stream in the connection's child_streams map. We could just close and drop the incoming side after the sender sends the 'end' packet. If they never send an 'end' packet, it'll hang around as garbage unless we give our read side the ability to tell the connection that it's totally done with the stream and it should be dropped, or we add a manual garbage collection step that scans child_streams for entries that no longer have a receiver attached.

Presumably for one-way streams, we can just close the stream immediately when either side sends an 'end' packet. If the source side sends 'end', it means they're done sending packets, and if the receiver side sends 'end' it means "please abort and i'm gonna ignore your future packets on this stream".

The connection's closed_streams set only needs to contain ids of incoming request streams (positive ids), so we know that an incoming packet with a positive id that isn't found in the child_streams map isn't a new request. Assuming that every implementation just keeps incrementing the id for each new request, we could probably just ignore packets with ids that aren't in the child_streams map and are <= the last received request id (ignoring the possibility of the id overflowing and restarting at 1, assuming that's what other implementations do after sending 2billion requests to the same peer)

@pietgeursen
Copy link
Member Author

@dominictarr can you shed any light on Sean's question above?

@sbillig
Copy link
Member

sbillig commented Mar 17, 2020

Did some minor cleanup, no big changes. Unfortunately, there's no way to tell what kind of stream is being created (source/sink/duplex) without looking at the body json. I'd prefer to avoid having to make any assumptions about the content of the body at this layer, but if we need to handle the receipt of an 'end' packet differently depending on the stream type, we either have to look at the json or let the higher layer control what happens.

There are 4 unused bits in the packet header, so there's plenty of space to store this info (and to differentiate between an error and a normal end for streams), but we'd always have to deal with legacy peers even if such a change were agreed upon.

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

Successfully merging this pull request may close these issues.

2 participants