Skip to content
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

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

Merged
merged 4 commits into from
Mar 18, 2024

Conversation

mat-sz
Copy link
Contributor

@mat-sz mat-sz commented Mar 2, 2024

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

Checklist

@mat-sz
Copy link
Contributor Author

mat-sz commented Mar 2, 2024

@mcollina As discussed here #289 (comment)

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.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please document how to get a WebSocketStream in the README?

@mcollina
Copy link
Member

mcollina commented Mar 3, 2024

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

@mat-sz
Copy link
Contributor Author

mat-sz commented Mar 4, 2024

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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Member

mcollina commented Mar 7, 2024

CI is failing

@mat-sz
Copy link
Contributor Author

mat-sz commented Mar 7, 2024

@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.

@mcollina mcollina merged commit bc969b7 into fastify:master Mar 18, 2024
19 checks passed
danielkutt added a commit to danielkutt/network-overrides that referenced this pull request Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants