-
Notifications
You must be signed in to change notification settings - Fork 30
fix: add per-connection stream limit #173
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,14 +2,33 @@ import type { Components } from '@libp2p/interfaces/components' | |
import type { StreamMuxerFactory, StreamMuxerInit } from '@libp2p/interfaces/stream-muxer' | ||
import { MplexStreamMuxer } from './mplex.js' | ||
|
||
export interface MplexInit extends StreamMuxerInit { | ||
export interface MplexInit { | ||
/** | ||
* The maximum size of message that can be sent in one go in bytes. | ||
* Messages larger than this will be split into multiple smaller | ||
* messages. | ||
*/ | ||
maxMsgSize?: number | ||
|
||
/** | ||
* 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. newbie question here: I assume it's not possible to just put the default here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, the interface is removed at compile time so it can't have default values. |
||
} | ||
|
||
export class Mplex implements StreamMuxerFactory { | ||
public protocol = '/mplex/6.7.0' | ||
private readonly init: MplexInit | ||
|
||
constructor (init: MplexInit = {}) { | ||
this.init = init | ||
} | ||
|
||
createStreamMuxer (components: Components, init?: MplexInit) { | ||
return new MplexStreamMuxer(components, init) | ||
createStreamMuxer (components: Components, init: StreamMuxerInit = {}) { | ||
return new MplexStreamMuxer(components, { | ||
...init, | ||
...this.init | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,9 +14,12 @@ import type { Components } from '@libp2p/interfaces/components' | |
import type { Sink } from 'it-stream-types' | ||
import type { StreamMuxer, StreamMuxerInit } from '@libp2p/interfaces/stream-muxer' | ||
import type { Stream } from '@libp2p/interfaces/connection' | ||
import type { MplexInit } from './index.js' | ||
|
||
const log = logger('libp2p:mplex') | ||
|
||
const MAX_STREAMS_PER_CONNECTION = 1024 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, how did we pick this default? How do we know it's a good value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
|
||
function printMessage (msg: Message) { | ||
const output: any = { | ||
...msg, | ||
|
@@ -38,9 +41,7 @@ export interface MplexStream extends Stream { | |
source: Pushable<Uint8Array> | ||
} | ||
|
||
export interface MplexInit extends StreamMuxerInit { | ||
maxMsgSize?: number | ||
} | ||
interface MplexStreamMuxerInit extends MplexInit, StreamMuxerInit {} | ||
|
||
export class MplexStreamMuxer implements StreamMuxer { | ||
public protocol = '/mplex/6.7.0' | ||
|
@@ -50,10 +51,10 @@ export class MplexStreamMuxer implements StreamMuxer { | |
|
||
private _streamId: number | ||
private readonly _streams: { initiators: Map<number, MplexStream>, receivers: Map<number, MplexStream> } | ||
private readonly _init: MplexInit | ||
private readonly _init: MplexStreamMuxerInit | ||
private readonly _source: { push: (val: Message) => void, end: (err?: Error) => void } | ||
|
||
constructor (components: Components, init?: MplexInit) { | ||
constructor (components: Components, init?: MplexStreamMuxerInit) { | ||
init = init ?? {} | ||
|
||
this._streamId = 0 | ||
|
@@ -122,6 +123,12 @@ export class MplexStreamMuxer implements StreamMuxer { | |
} | ||
|
||
_newStream (options: { id: number, name: string, type: 'initiator' | 'receiver', registry: Map<number, MplexStream> }) { | ||
const maxStreams = this._init.maxStreamsPerConnection ?? MAX_STREAMS_PER_CONNECTION | ||
|
||
if ((this._streams.initiators.size + this._streams.receivers.size) === maxStreams) { | ||
throw errCode(new Error('To many streams open'), 'ERR_TOO_MANY_STREAMS') | ||
} | ||
|
||
const { id, name, type, registry } = options | ||
|
||
log('new %s stream %s %s', type, id, name) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
/* eslint-env mocha */ | ||
/* eslint max-nested-callbacks: ["error", 5] */ | ||
|
||
import { expect } from 'aegir/chai' | ||
import { Mplex } from '../src/index.js' | ||
import { Components } from '@libp2p/interfaces/components' | ||
import type { NewStreamMessage } from '../src/message-types.js' | ||
import { fromString as uint8ArrayFromString } from 'uint8arrays/from-string' | ||
import { concat as uint8ArrayConcat } from 'uint8arrays/concat' | ||
import { encode } from '../src/encode.js' | ||
import all from 'it-all' | ||
|
||
describe('mplex', () => { | ||
it('should restrict number of initiator streams per connection', async () => { | ||
const maxStreamsPerConnection = 10 | ||
const factory = new Mplex({ | ||
maxStreamsPerConnection | ||
}) | ||
const components = new Components() | ||
const muxer = factory.createStreamMuxer(components) | ||
|
||
// max out the streams for this connection | ||
for (let i = 0; i < maxStreamsPerConnection; i++) { | ||
muxer.newStream() | ||
} | ||
|
||
// open one more | ||
expect(() => muxer.newStream()).to.throw().with.property('code', 'ERR_TOO_MANY_STREAMS') | ||
}) | ||
|
||
it('should restrict number of recipient streams per connection', async () => { | ||
const maxStreamsPerConnection = 10 | ||
const factory = new Mplex({ | ||
maxStreamsPerConnection | ||
}) | ||
const components = new Components() | ||
const muxer = factory.createStreamMuxer(components) | ||
|
||
// max out the streams for this connection | ||
for (let i = 0; i < maxStreamsPerConnection; i++) { | ||
muxer.newStream() | ||
} | ||
|
||
// simulate a new incoming stream | ||
const source: NewStreamMessage[] = [{ | ||
id: 17, | ||
type: 0, | ||
data: uint8ArrayFromString('17') | ||
}] | ||
|
||
const data = uint8ArrayConcat(await all(encode(source))) | ||
|
||
await muxer.sink([data]) | ||
|
||
await expect(all(muxer.source)).to.eventually.be.rejected.with.property('code', 'ERR_TOO_MANY_STREAMS') | ||
}) | ||
}) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, generating docs from the interface might be a better approach here.