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

fix eventemitter madness, be careful around pausing here too #206

Closed
wants to merge 2 commits into from

Conversation

quartzjer
Copy link

Much happier :)

@indexzero
Copy link
Contributor

@quartzjer This looks good. So good that I would ask that you update other places you see flushed = *.write(... with this pattern.

@quartzjer
Copy link
Author

Ok, I honestly tried to fix this elsewhere, but I got in the weeds quickly on knowing what kind of larger impact it would have, I don't know the internals of http-proxy well enough to have any confidence in twiddling these bits. It seems that for all the instances of tracking flushed, paused, and attaching drain listeners, that there needs to be some way to check if the drain listener is attached for-the-specific-paired-socket, and using a paused variable could actually result in some edge cases of a full standstill.

I'm not sure how to do this properly in all the other places with my relatively shallow understanding of http-proxy, and may not have the time to do a deep dive and learn it all any time soon :(

cc @temas

@quartzjer
Copy link
Author

I decided to go the safest route possible, and just have a variable per instance. I tested this everywhere we were seeing the event emitter leak on the locker project and it runs clean now. Should be good to go I think?

@quartzjer
Copy link
Author

Does the current patch look good?

@indexzero
Copy link
Contributor

Things have diverged so much from this that I can't merge it in. If you encounter this in v0.9.0 when it is released let us know. Sorry.

@indexzero indexzero closed this Mar 9, 2013
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

3 participants