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

Make Receiver inherit from stream.Writable #1302

Merged
merged 3 commits into from Mar 1, 2018
Merged

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Feb 15, 2018

This makes Receiver inherit from stream.Writable.

The main reason for this change is to have back-pressure properly handled when permessage-deflate is enabled.

There are no significant performance differences but memory usage is a lot higher. 100k connections use ~400 MB on master and ~600 MB with this patch.

I'm opening this PR to discuss whether or not this makes sense.

@lpinca lpinca force-pushed the subclass/stream-writable branch 3 times, most recently from 418431d to 99018e6 Compare February 15, 2018 15:47
@lpinca
Copy link
Member Author

lpinca commented Feb 19, 2018

Managed to reduce memory consumption by not using Readable.prototype.pipe(). Now 100k connections use ~450 MB which is ~10% more memory than master.

I think this is a good trade off.

@lpinca
Copy link
Member Author

lpinca commented Feb 21, 2018

If there is anyone watching this, can you please share your opinion about net.Socket errors? Should we re-emit them or not?

  • Node.js core is not consistent about these errors on the HTTP stack. On the server they are not re-emitted, or better they are re-emitted using the 'clientError' event on the server but you can omit the 'error' listener. On the client they are re-emitted on the ClientRequest instance and you can't omit the 'error' listener.
  • From read ECONNRESET error on disconnect (ws 3.3.3) #1256 it seems that these errors end up being ignored as there is not much that can be done to prevent them. If the common action is to filter them out, I think there is no point in re-emitting them as the WebSocket is closed anyway with an abnormal closure status code.
  • The WebSocket browser implementation does not call the onerror listener if a network error occurs after the opening handshake completes.

cc: @websockets/admin

@lpinca
Copy link
Member Author

lpinca commented Feb 26, 2018

This library has ~15M monthly downloads, it's a dependency of many popular projects, it's used by many big corporations but almost no one actively participate in its development. This is weird.

@3rd-Eden can you please weigh in when you have time?

@3rd-Eden
Copy link
Member

I simply do not know enough of Node.js net code internal to make a comment that will be remotely useful here.

The performance loss is acceptable as we finally handle back pressure correctly. We can see if we can performance back in different ways.

@lpinca
Copy link
Member Author

lpinca commented Feb 28, 2018

Performance is the same, it's memory usage that's slightly higher but that's acceptable.

My doubt is whether or not we should re-emit network errors like ECONNRESET, EPIPE, ECONNABORTED. All of these were swallowed before version 3.3.3 and from #1256 it seems that the common action is to ignore them.

@3rd-Eden
Copy link
Member

@lpinca I agree, I don't think those errors are usefull because it just means that the connection got closed in some way.

@lpinca
Copy link
Member Author

lpinca commented Feb 28, 2018

Ok, thank you.

@lpinca lpinca merged commit a4050db into master Mar 1, 2018
@lpinca lpinca deleted the subclass/stream-writable branch March 1, 2018 11:26
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.

None yet

2 participants