-
Notifications
You must be signed in to change notification settings - Fork 33
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(hydroflow): cleanup temp tcp networking code, fix race condition fix #1458 #1446
base: main
Are you sure you want to change the base?
Conversation
2ce671b
to
a977564
Compare
bced4ad
to
99846df
Compare
hydro-project#1458 only spawn one task to prevent races between tasks
99846df
to
006890f
Compare
@@ -74,6 +73,7 @@ pub type TcpFramedSink<T> = Sender<(T, SocketAddr)>; | |||
pub type TcpFramedStream<Codec: Decoder> = | |||
Receiver<Result<(<Codec as Decoder>::Item, SocketAddr), <Codec as Decoder>::Error>>; | |||
|
|||
// TODO(mingwei): this temporary code should be replaced with a properly thought out networking system. |
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.
Indeed.
continue; | ||
// Calling methods in a loop, futures must be cancel-safe. | ||
select! { | ||
biased; |
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.
It will be helpful to document why biased
is necessary.
// Send outgoing messages. | ||
msg_send = recv_egress.next() => { | ||
let Some((payload, peer_addr)) = msg_send else { | ||
continue; |
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.
In what condition is this continue
called?
} | ||
} | ||
}); | ||
let (send_egress, mut recv_egress) = unsync_channel::<(T, SocketAddr)>(None); |
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.
Curious, what was wrong with tx
and rx
?
loop { | ||
// Calling methods in a loop, futures must be cancel-safe. | ||
select! { | ||
biased; |
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.
Ditto on why biased
is necessary.
Nice. Mostly LGTM, just a few questions for clarity. |
consolidate into one task to prevent races