-
Notifications
You must be signed in to change notification settings - Fork 30
Conversation
License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
Thanks @vasco-santos! License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
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.
Just some small comments. I am running the adapter through the benchmarks at https://github.com/libp2p/pull-mplex/tree/master/benchmarks to see what the difference is. I'm hitting some issues with the adapter benchmarks, I'm looking into it. Right now the bench test is really slow and the large file test hangs.
Co-Authored-By: Jacob Heun <jacobheun@gmail.com>
License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
@alanshaw the restrictSize fix took care of the issue with benchmarks. When I was running that the restrict size error ends up getting swallowed and never logged, only abort errors show in the logs. Can we have the source errors propagate up to the logs, they seem to be getting swallowed? |
I'll look into it - this still needs work clearly. The slow pingpong benchmark is also worrying. |
License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
I used npm alias support to install the interface-stream-muxer@0.6 code along side the async test suite to test the adapter, as I was curious about the close tests. It's failing the close tests there as well (both close tests fail for the adapter), so I think there is something wrong with error/close propagation. |
@jacobheun I'm working on a fork of the |
License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
src/coder/decode.js
Outdated
try { | ||
this._headerInfo = this._decodeHeader(this._bufferProxy) | ||
} catch (_) { | ||
break // not enough data yet...probably |
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.
Can you elaborate a bit more? The "probably" is not really comforting :)
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.
Updated the comment for this. We should only hit the break in 2 instances.
- We haven't gotten enough data to parse a header (most common).
- The data we have can't be parsed, likely due to an invalid varint from bad data.
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've added some comments/jsdocs to the code, removed the adapter (we dont need it), and fixed an issue with message logging. This should be all set.
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 was trying out the examples and I am having issues with the dialer, not sure why
Fixed, the BufferList chunks need to be converted before going to node streams .write |
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.
LGTM!
@jacobheun this is https://github.com/alanshaw/it-mplex + the example from this repo updated to use the new API.
Note this includes an adapter class so it can be used immediately in JS IPFS when merged and released. I've run the JS IPFS tests and interop tests against it and they pass!
TODO:
Depends on:
BREAKING CHANGE: This module now has a completely new API based on async iterators. See the README for documentation.