generated from MetaMask/metamask-module-template
-
Notifications
You must be signed in to change notification settings - Fork 6
Do Not Merge: Stream multiplexing epic #218
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
Closed
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Nov 6, 2024
4d4cf0a to
004e27e
Compare
rekmarks
added a commit
that referenced
this pull request
Nov 6, 2024
Ref #218 Adds consumer-provided input validation to streams in the form of a function constructor argument. This is necessary in order for streams to actually enforce the types they claim to read and write. Input validation only occurs in read streams, because we can simply rely on TypeScript to write the correct values. If input validation fails, the read stream will end.
0618b9f to
e6a7cd6
Compare
rekmarks
added a commit
that referenced
this pull request
Nov 6, 2024
Ref: #218 Add name param to all stream base classes, for logging purposes. Also update `BaseWriter` constructor params to options bag.
Also tidy up various asynchronous operations in base stream classes. These were causing promises returned from the stream multiplexer to fulfill erratically.
Get rid of the `makeChannels()` contrivance in favor of exposing a singular `addChannel()`. Also expose `start()` function in order to enable either "manually" draining channels or using the `drainAll()` method.
Also make `synchronize()` method of duplex streams optional.
004e27e to
651cce1
Compare
Also fix issues in tests following rebase.
c6c8dff to
2933d09
Compare
cf2a25b to
6992b7e
Compare
6992b7e to
32a2722
Compare
Closed
Closed
Closed
rekmarks
added a commit
that referenced
this pull request
Nov 7, 2024
Ref: #218 Adds a new class `StreamMultiplexer` to `@ocap/streams`. A multiplexer is not a stream itself, but rather a wrapper around a duplex stream. The multiplexer provides methods for creating "channels" over the underlying stream, which are themselves duplex streams and may have a different message type and validation logic. The multiplexer is constructed in an idle state, and must be explicitly "started" via the `start()` or `drainAll()` methods. All channels must be added before the multiplexer is started. Starting the multiplexer will synchronize the underlying duplex stream, if it is synchronizable. Therefore, in order to prevent message loss, callers **should not** synchronize the underlying duplex stream before passing it to the multiplexer. For the same reason, the multiplexer will throw if any channels are added after it has started. We could enable "dynamic" channel creation by synchronization the channels themselves, but only if there's a clear need. See tests for example usage.
Removes the JSON constraint for our stream types. Leaves it in place for Chrome runtime streams, since they actually require this constraint. Now we are free to send non-JSON data over transports that can handle it.
rekmarks
added a commit
that referenced
this pull request
Nov 7, 2024
Ref: #218 Replaces stream envelopes with multiplex streams. This simplifies multiplexing traffic over the same underlying transport.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TODODefault channels toJsonfor read/write and no / trivial input validationFuture TODO