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

propagating end events to server #57

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

soldair
Copy link
Contributor

@soldair soldair commented Aug 15, 2014

fixes. Leaks all client ended read streams.

readable streams passed from server to client like level-live-stream that never end on the server cannot be ended from multilevel client. the server never gets the end event though they are supported by mdm and the client ends them.

the fix is when a tmp-stream is "replaced" with the mdm stream bind end on the tmp stream to trigger end on the mdm stream. for write stream support also close should be bubbled.

this example results in the whole database being streamed to the client. you can see it if you bind "data" on the mdm stream to console log or just look at your server's network traffic graph.

db.createReadStream().once('data',function(){ this.end(); })

the expected behavior is that the server side of the stream gets the end event that mdm passes and stops streaming the whole database.

@soldair
Copy link
Contributor Author

soldair commented Aug 15, 2014

this bug is super duper bad in my case because i have a db server process that many api server processes connect to with a single long lived connection. Any api client request that results in a live stream leaks the hook in the db server process and maxes out process rather quickly.

@soldair soldair changed the title propagating close and end events to server propagating end events to server Aug 15, 2014
thats what i get for not copy pasting the tested fix...
@juliangruber
Copy link
Owner

i'm looking at this now...progress! :D

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.

2 participants