Skip to content

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

Merged
mcollina merged 4 commits intofastify:masterfrom
mat-sz:master
Mar 18, 2024
Merged

fix: provide websocket instead of stream to avoid potential backpressure issues (#289)#290
mcollina merged 4 commits intofastify:masterfrom
mat-sz:master

Conversation

@mat-sz
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

mat-sz commented Mar 2, 2024 •
edited
Loading

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

@mat-sz mat-sz mentioned this pull request Mar 2, 2024
Merged
3 tasks
mcollina
mcollina requested changes Mar 3, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

mat-sz commented Mar 4, 2024

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

mcollina
mcollina approved these changes Mar 5, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

mcollina commented Mar 7, 2024

CI is failing

@mat-sz
Copy link
Copy Markdown
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
danielkutt added a commit to danielkutt/network-overrides that referenced this pull request Oct 21, 2024
@danielkutt
Bump dependencies to latest-ish
e65a777 
v10 of websocket required refactor:
fastify/fastify-websocket#290
@danielkutt danielkutt mentioned this pull request Oct 21, 2024
Merged
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 1, 2026

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators May 1, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mcollina mcollina mcollina approved these changes

@gurgunday gurgunday gurgunday approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mat-sz @mcollina @gurgunday