Fixes to the mplex implementation #360
Conversation
ghost
assigned
|
Fixed a memory leak with outgoing substreams. |
gnunicorn
left a comment
gnunicorn
left a comment
There was a problem hiding this comment.
Thanks. Minor changes requested. Feel free to merge after having fixed them.
mplex/src/lib.rs
Outdated
| F: FnMut(&codec::Elem) -> Option<O>, | ||
| { | ||
| // If an error happened earlier, immediately return it. | ||
| match inner.error { |
There was a problem hiding this comment.
maybe make it more explicit rather than having an empty match?
if let Err(ref err) = inner.error {
return Err(IoError::new(err.kind(), err.to_string()))
}| })); | ||
|
|
||
| if let Some(num) = num { | ||
| inner.opened_substreams.insert(num); |
There was a problem hiding this comment.
whty don't we insert this any longer?
There was a problem hiding this comment.
It's dealt with in next_match.
|
tests failing?!? |
|
CI fails because of issues fixed by #366. |
mplex/src/lib.rs
Outdated
| error: Result<(), IoError>, | ||
| // Underlying stream. | ||
| inner: Fuse<Framed<C, codec::Codec>>, | ||
| inner: executor::Spawn<Fuse<Framed<C, codec::Codec>>>, |
There was a problem hiding this comment.
Could you explain a bit what made this change and the introduction of Notifier necessary?
There was a problem hiding this comment.
To be honest I thought it was necessary for multiplexing to work correctly, then realized it was not.
|
I reverted the switch to spawning a new task, as it is indeed unnecessary. |
3 participants
Substreamstruct yet, which is bad.