-
Notifications
You must be signed in to change notification settings - Fork 840
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix(websocket): fixes websocket upgrade issue (#82)
- Loading branch information
Showing
1 changed file
with
5 additions
and
12 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
53fe79b
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.
This creates a leak for me with ws: true.
Every request seems to add an upgrade handler on the server. After 10 requests I get:
53fe79b
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.
Can you share your server+proxy configuration?
53fe79b
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.
Yes, the server is browser-sync, I'm also hitting #15. I discovered the leak trying to debug what's going wrong there.
Here is my proxy config:
And here is the server config:
53fe79b
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.
Hi @julbra.
As you already discovered, there is still no solution for browser-sync and websockets proxy.
Wondering if this memory leak issue is isolated to browser-sync or is it also reproducable in
express
orconnect
?53fe79b
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.
Just tested the example setup
http-proxy-middleware/examples/websocket
with 20 active connections without issues.This issue is probably similar to #108
Would be great if you can create a minimal working setup with
express
orconnect
in which the issue is exposed. (http://stackoverflow.com/help/mcve)53fe79b
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.
@chimurai I created a minimal project that reproduces the bug in browser-sync:
Clone from https://github.com/julbra/http-proxy-middleware-leak-test.git
npm install then run gulp
wget http://localhost:3100/ 10 times
After the 10th wget I get the warning.
Will test using two express instances to see if the bug is still there without browser-sync
53fe79b
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.
@chimurai I updated https://github.com/julbra/http-proxy-middleware-leak-test with an express only task.
The same bug is also found there without browser-sync involved.
53fe79b
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.
Hi @julbra.
Good news. Think I've found the issue.
I created a fix in PR #114. (Also created a separate issue #113 to track the issue)
Can you verify if this fixes the memory leak? (You only need to replace 1 file.)
53fe79b
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.
@chimurai fixes the bug, ship it ;)