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
Conversation
418431d
to
99018e6
Compare
Managed to reduce memory consumption by not using I think this is a good trade off. |
c839044
to
c4acce8
Compare
c4acce8
to
f54320c
Compare
If there is anyone watching this, can you please share your opinion about
cc: @websockets/admin |
f54320c
to
908c358
Compare
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? |
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. |
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 |
@lpinca I agree, I don't think those errors are usefull because it just means that the connection got closed in some way. |
Ok, thank you. |
This makes
Receiver
inherit fromstream.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.