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

fix: fix some msg left on buffer #288

Merged
merged 2 commits into from
Dec 31, 2020
Merged

fix: fix some msg left on buffer #288

merged 2 commits into from
Dec 31, 2020

Conversation

driftluo
Copy link
Collaborator

  1. sink start_send just set data to buffer, flush will try to write data to inner io
  2. Remove write buffer on yamux stream, it will cause context confusion, and let the data be copied once more
  3. yamux stream read must break on having read something
  4. yamux control message send by unbound channel
  5. secio after confirming that it can be sent, encrypt the data

codec write buffer left:

https://github.com/nervosnetwork/tentacle/blob/fix-message-delay/tentacle/src/substream.rs#L427

yamux read buffer left:

https://github.com/nervosnetwork/tentacle/blob/fix-message-delay/yamux/src/stream.rs#L302

@driftluo driftluo requested a review from TheWaWaR as a code owner December 23, 2020 06:26
@driftluo driftluo requested review from a team and xxuejie December 23, 2020 06:26
@driftluo driftluo force-pushed the fix-message-delay branch 2 times, most recently from 8b17626 to 520a358 Compare December 23, 2020 08:06
@driftluo driftluo mentioned this pull request Dec 24, 2020
Copy link
Member

@doitian doitian left a comment

Choose a reason for hiding this comment

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

Add some test cases?

@@ -176,11 +175,11 @@ where
}

fn poll_flush(mut self: Pin<&mut Self>, cx: &mut Context) -> Poll<io::Result<()>> {
Pin::new(&mut self.socket).as_mut().poll_flush(cx)
self.socket.poll_flush_unpin(cx)
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What's the difference?

No difference, may look better

@driftluo
Copy link
Collaborator Author

Add some test cases?

There have been tests, and occasionally failed, but did not pay attention to it, thought it was a problem caused by poor ci machine

@@ -82,6 +82,9 @@ pub struct Session<T> {
event_sender: Sender<StreamEvent>,
// For receive events from sub streams
event_receiver: Receiver<StreamEvent>,
// The control information must be sent successfully and will not cause memory problems
unbound_event_sender: UnboundedSender<StreamEvent>,
Copy link
Member

Choose a reason for hiding this comment

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

unbounded is dangerous, giving a large enough bound channel is better.

Copy link
Collaborator Author

@driftluo driftluo Dec 29, 2020

Choose a reason for hiding this comment

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

This channel is only used to send control information(window update and close), there is no possibility of unlimited increase

Copy link

@yangby-cryptape yangby-cryptape left a comment

Choose a reason for hiding this comment

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

There is an integration test in CKB fails randomly, and it may causes by this issue.
I ran it again and again for more than 4 hours with this patch, it didn't fail.
So, I approved this pull request.

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.

4 participants