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

feat: uid-mux #28

Merged
merged 10 commits into from
May 24, 2024
Merged

feat: uid-mux #28

merged 10 commits into from
May 24, 2024

Conversation

sinui0
Copy link
Member

@sinui0 sinui0 commented May 14, 2024

This PR moves/reimplements uid-mux in this repo.

Changes

  • Uses newer version of yamux which has better flow control. This should give us higher throughput.
  • Stream ids are now hashed to a fixed-length
  • Better logging
  • Cleaner.

@sinui0 sinui0 requested review from themighty1 and th4s May 14, 2024 19:17
@sinui0
Copy link
Member Author

sinui0 commented May 14, 2024

This is supporting work to implement a multi-threaded executor in mpz

@sinui0
Copy link
Member Author

sinui0 commented May 15, 2024

Sigh, hold off on review. There is a deadlock somewhere

@sinui0
Copy link
Member Author

sinui0 commented May 15, 2024

It is seeming to me that yamux may have a bug, hard to say conclusively. It looks like their implementation is letting data sit in the write buffer with no way to flush it.

@sinui0
Copy link
Member Author

sinui0 commented May 15, 2024

Nope, nevermind there is a way to flush it. Fix coming soon

@sinui0
Copy link
Member Author

sinui0 commented May 15, 2024

@th4s @themighty1 ready!

Copy link
Member

@themighty1 themighty1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gw!!
Some comments, potentially blocking.

Also, could you pls adjust the comments acc.to the style guide to be /// Create --> Creates etc.
I'll open a PR to add the guide to this repo.

uid-mux/src/yamux.rs Outdated Show resolved Hide resolved
uid-mux/src/yamux.rs Show resolved Hide resolved
uid-mux/src/yamux.rs Outdated Show resolved Hide resolved
uid-mux/src/yamux.rs Outdated Show resolved Hide resolved
Copy link
Member

@th4s th4s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice and clean 🚀

There is one thing I do not get. There has to be some future running in order to transmit the data between client and server on existing streams. But currently it looks to me that in YamuxFuture::poll in the end we return Poll::Pending. So there has to be some way to wake it again, so that again data is transmitted.

But the only place we wake it, is when opening a stream (and when closing the connection). This to me sounds like data on existing streams is only transmitted when a new stream is opened or the connection is closed.

So what I mean is that yamux::Connection::poll_next_inbound is not permanently polled.

EDIT: After some more investigation, I now suspect that it works correctly, because yamux::Connection::Active contains a loop in its poll method.

uid-mux/src/lib.rs Show resolved Hide resolved
uid-mux/src/yamux.rs Show resolved Hide resolved
uid-mux/src/yamux.rs Show resolved Hide resolved
uid-mux/src/yamux.rs Show resolved Hide resolved
uid-mux/src/yamux.rs Outdated Show resolved Hide resolved
@sinui0
Copy link
Member Author

sinui0 commented May 17, 2024

@th4s

But the only place we wake it, is when opening a stream (and when closing the connection)

Anytime a Future returns Poll::Pending it (should) clone the Waker provided by Context and ensure that it is triggered when it is ready to be polled again. All the polls of the underlying yamux connection are doing this, so they contribute to driving YamuxFuture in addition to YamuxCtrl

@sinui0
Copy link
Member Author

sinui0 commented May 17, 2024

@th4s @themighty1 it seems my understanding of Waker was not complete. Apparently it is re-usable across multiple Future:poll calls and does not need to be cloned multiple times. This is nice and simplifies things a bit.

Edit: Nevermind, ChatGPT lied to me! 😂

Note that on multiple calls to poll, only the Waker from the Context passed to the most recent call should be scheduled to receive a wakeup.

@sinui0 sinui0 requested review from th4s and themighty1 May 17, 2024 16:47
uid-mux/src/yamux.rs Outdated Show resolved Hide resolved
uid-mux/src/yamux.rs Outdated Show resolved Hide resolved
@themighty1
Copy link
Member

There is a race condition iiuc which violates this requirement: "Note that on multiple calls to poll, only the Waker from the Context passed to the most recent call should be scheduled to receive a wakeup"

Suppose that the future has just been polled and the execution is somewhere around this line

fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
match self.role {

If around the same time the YamuxCtrl::close() call happens and tries to get the waker in

.map(|waker| waker.wake_by_ref());

it will get the waker from the previous context, not from the current context as per the requirement.

@sinui0
Copy link
Member Author

sinui0 commented May 21, 2024

That would be fine, as the future would already be getting polled. What matters is that the waker is reset before the future returns Poll::Pending, which it does on every poll. The risk of waking a stale waker is that the future never gets polled again and you get a deadlock.

@themighty1 themighty1 self-requested a review May 24, 2024 06:28
Copy link
Member

@themighty1 themighty1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

* feat(uid-mux): framed mux

* add test utils

* Apply suggestions from code review

Co-authored-by: dan <[email protected]>

---------

Co-authored-by: dan <[email protected]>
@sinui0 sinui0 merged commit 6cf35e0 into dev May 24, 2024
2 checks passed
@sinui0 sinui0 deleted the feature/id-mux branch May 24, 2024 20:20
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.

3 participants