fix: add per-connection stream limit #173
Conversation
Limits the total number of streams that can be opened to 1024 per connection. This can be overridden by passing an option to the constructor.
README.md
Outdated
|
|
||
| `options` is an optional `Object` that may have the following properties: | ||
|
|
||
| * `maxMsgSize` - a number that defines how large mplex data messages can be in bytes, if messages are larger than this they will be sent as multiple messages (default: 1 << 20) |
There was a problem hiding this comment.
minor thing, i think describing the default as a power of 2 is more readable (eg: 2 ** 20)
or just writing the number
### [1.1.2](v1.1.1...v1.1.2) (2022-06-08) ### Bug Fixes * add per-connection stream limit ([#173](#173)) ([21371e7](21371e7))
|
🎉 This PR is included in version 1.1.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
|
||
| const log = logger('libp2p:mplex') | ||
|
|
||
| const MAX_STREAMS_PER_CONNECTION = 1024 |
There was a problem hiding this comment.
newbie question: is the TypeScript way of doing things to define defaults here rather than where the override can be set? For example, I was surprised not to see this closer to MplexInit.
There was a problem hiding this comment.
Also, how did we pick this default? How do we know it's a good value?
There was a problem hiding this comment.
MplexInit is defined in index.ts, exporting it for import into mplex.ts would also expose it to people importing @libp2p/mplex. It could be the other way round though, defined in mplex.ts and imported into index.ts.
There was a problem hiding this comment.
Also, how did we pick this default?
Completely arbitrary. It's a lot bigger than go-libp2p but we need to start somewhere. We might end up splitting it into separate incoming/outgoing stream limits too.
| * The maximum number of multiplexed streams that can be open at any | ||
| * one time. An attempt to open more than this will throw. | ||
| */ | ||
| maxStreamsPerConnection?: number |
There was a problem hiding this comment.
newbie question here: I assume it's not possible to just put the default here?
There was a problem hiding this comment.
No, the interface is removed at compile time so it can't have default values.
| `options` is an optional `Object` that may have the following properties: | ||
|
|
||
| * `maxMsgSize` - a number that defines how large mplex data messages can be in bytes, if messages are larger than this they will be sent as multiple messages (default: 1048576 - e.g. 1MB) | ||
| * `maxStreamsPerConnection` - a number that defines how many streams are allowed per connection (default: 1024) |
There was a problem hiding this comment.
The default values listed in these docs can get out of date. Can/should we link to the authoritative source?
There was a problem hiding this comment.
Sure, generating docs from the interface might be a better approach here.
Limits the total number of streams that can be opened to 1024 per connection.
This can be overridden by passing an option to the constructor.