fix: provide websocket instead of stream to avoid potential backpressure issues (#289)

fastify / fastify-websocket

basic websocket support for fastify

MIT License

394 stars 72 forks source link

fix: provide websocket instead of stream to avoid potential backpressure issues (#289) #290

Closed mat-sz closed 6 months ago

mat-sz commented 6 months ago

Now createWebSocketStream is not called therefore there are no memory leaks/backpressure to worry about.

Checklist

[x] run npm run test and npm run benchmark
[x] tests and/or benchmarks are included
[x] documentation is changed or added
[x] commit message and code follows the Developer's Certification of Origin and the Code of conduct

mat-sz commented 6 months ago

@mcollina As discussed here https://github.com/fastify/fastify-websocket/issues/289#issuecomment-1973621926

I've removed stream creation completely. Another option here is to provide an object of shape { socket: WebSocket } to provide compatibility with most common use cases.

Also, could be nice to provide a function to convert that socket into a stream; so the object could even be { socket: WebSocket; getStream: (opts?: DuplexOptions) => Promise<Duplex> }.

I can update this PR to include either of the aforementioned solutions.

mcollina commented 6 months ago

I don't think having a specific function is needed, let's just document how to create it using ws.

mat-sz commented 6 months ago

@mcollina I've added an example to README.md

mcollina commented 6 months ago

CI is failing

mat-sz commented 6 months ago

@mcollina Resolved, for some reason the unit test was relying on the count of fastify log items to be constant, updated it to fail if level >= 50 instead.