Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add a Stream Migration spec #406
base: master
Are you sure you want to change the base?
Add a Stream Migration spec #406
Changes from 6 commits
4fc768f
b0db416
dac9a8d
37a86ff
6f03926
406171f
99f055e
c0fd69f
eef233e
49c0597
4e1f106
a380dc4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the peer misbehaves, e.g. when B just doesn’t send an EOF on stream B? While A can “consider” the stream as closed, it still needs to be closed explicitly, otherwise the stream multiplexer can’t garbage-collect the stream after it has been EOFed from both sides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if the Responder misbehaves and doesn't close B then the stream migration hasn't finished since the new stream isn't in the same state as the old stream. I'm not sure what else we can do besides consider it not spec-compliant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Below I am assuming that Initiator refers to the connection initiator, not the stream initiator. Please correct me in case I am misunderstanding this @MarcoPolo.
Say there are two nodes
A
andB
. ConnectionAB
is initiated byA
toB
. ConnecitonBA
is initiated byB
toA
.Say that
A
andB
follow different heuristics to pick the best connection.A
choosesAB
as the best connection,B
choosesBA
as the best connection.If I understand the above correctly, this would result in
A
moving all the streams it created toAB
andB
moving all the streams it created toBA
. Both connections would thus stay alive.Potential solution: Instead of allowing both
A
andB
to migrate streams, how about delegating the decision making to the peer with the lower peer ID, e.g. in this caseA
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
I was thinking about this this weekend and came up with a similar solution. Glad to see you also came to the same conclusion. I'll update this spec to make this explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few thoughts on this:
AB
overBA
. This document should only describe how to migrate one libp2p stream from one muxer stream to another. For all that this spec cares about, those streams might (or might not) live on the same underlying connection. We can then use the stream migration protocol as a building block to converge onto one connection (and the peer ID comparison is quite a neat idea, I like it!), but that should probably be described in a different document.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I was considering adding a boolean flag that would indicate which peer initially identified the stream it's referencing when migrating (was I the initiator of the from stream?). This is the same as using even and odd numbers, since that scheme effectively encodes this boolean in the least significant bit. I'm fine with either way. Maybe it's a little easier to think about even and odds, so I'll do that.
Agreed that describing how to sort connections is out of the scope of this document (I imagine that spec to iterate more and and possibly have more subtle details). But I do think this spec should define who is responsible for doing the stream migration. If we end up in the situation where we have two identical connections (
A
dialedB
andB
dialedA
at roughly the same time) we should describe who is in charge of doing the stream migration. By defining which node starts the stream migration we simplify this protocol and also avoid having to handle cases where both sides start stream migration at the same time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Potential solution: Instead of allowing both A and B to migrate streams, how about delegating the decision making to the peer with the lower peer ID, e.g. in this case A?"
Wouldn't this create a biais toward lower peerIDs? Maybe we can hash the concatenation of the two peerIDs (the lower first). If the hash is even, use the lowest. Else use the highest. That way it is deterministic but no peerID is systematically favored over another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's not worth the extra complexity though since for a random ID
A
there's a 50% odd that it's less than another random idB
. (since it's equiprobable thatB
is smaller).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah on second thought I agree with Max. I actually don't see the benefits since it's still 50% odds either way. Updated 49c0597
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not convinced that we should specify anything here at all. Stream migration is a general feature, a building block.
The use case we have in mind now is migrating all streams from one connection to another, but we might come up with other use cases in the future. I'd prefer to have stream migration just be a thing that any node, regardless of its peer ID, can use in principle.
For the specific use case of converging on a single connection, comparing peer IDs seems reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me try to rephrase to see if I understand:
We just have to make sure that what we design here doesn't block point 3.
If that seems accurate, then I agree we don't need this in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also @marten-seemann to highlight a recent change re:
I've specced something similar, except the lower peer id node uses even and the higher peer id node uses odds. This let's us avoid having to rely on the stream muxer to give us this role. And it also works across connections (it gets confusing if the stream muxer says we are the client on connection and the server on the other).