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.

Sign up for GitHub

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

Jump to bottom

Revert "Meh…" #79

Merged
merged 2 commits into from
Oct 4, 2019
Merged

Revert "Meh…" #79

merged 2 commits into from
Oct 4, 2019

Conversation

lpinca
Copy link
Contributor

@lpinca lpinca commented Oct 4, 2019

This reverts commit 2590f76 as per 2590f76#commitcomment-35365193

@lpinca
Revert "Meh…"
30ca5b6 
This reverts commit 2590f76.
@TooTallNate
Copy link
Owner

The reason that I did that commit was because (on macOS, at least, but apparently not Linux) the "after all" hooks were timing out (the proxy and sslProxy never emit a "close" event).

If you would like to dig deeper into why macOS is timing out that would be appreciated.

@lpinca
Copy link
Contributor Author

lpinca commented Oct 4, 2019

Hmm I'm on macOS (10.14.6) and I see no issues. I'll add it to Travis.

@lpinca
Copy link
Contributor Author

lpinca commented Oct 4, 2019 •
edited
Loading

There are some failing tests on macOS but no timeouts. It seems something is written to the socket after it is destroyed by the other peer.

@TooTallNate
Copy link
Owner

The tests failing seems to be intermittent. And indeed it's working properly for me now on macOS. Not sure what the issue for me was last night.

@TooTallNate TooTallNate merged commit 5252bb9 into TooTallNate:master Oct 4, 2019
@lpinca
Copy link
Contributor Author

lpinca commented Oct 4, 2019

I think the failing tests are due to an 'error' listener removed too soon but I'm not sure if here or the proxy dependency.

@lpinca lpinca deleted the revert/2590f76d branch October 4, 2019 20:17
@lpinca
Copy link
Contributor Author

lpinca commented Oct 6, 2019

I investigated a little and it seems tests flakiness on macOS is caused by the premature removal of the 'error' listener on the client socket done by the proxy server on this line https://github.com/TooTallNate/proxy/blob/1.0.0/proxy.js#L319. socket.end() sometimes raises an error ('read ECONNRESET').

Interestingly the error is only emitted when using an HTTPS proxy and the target is an HTTPS endpoint but I did not figure out why.

lpinca added a commit to lpinca/proxy that referenced this pull request Oct 6, 2019
@lpinca
Do not remove the event listeners of the sockets
438ab9a 
Sockets are not reused so the listeners should be GC'ed when the sockets
are closed. Furthermore there are cases where the `'error'` listeners
are prematurely removed leading to unhandled errors and unexpected
crashes.

Refs: TooTallNate/proxy-agents#79
@lpinca
Copy link
Contributor Author

lpinca commented Oct 6, 2019

I've opened TooTallNate/proxy#16.

TooTallNate pushed a commit to TooTallNate/proxy that referenced this pull request Oct 7, 2019
@lpinca @TooTallNate
Do not remove the event listeners of the sockets (#16)
2020b5e 
* Do not remove the event listeners of the sockets

Sockets are not reused so the listeners should be GC'ed when the sockets
are closed. Furthermore there are cases where the `'error'` listeners
are prematurely removed leading to unhandled errors and unexpected
crashes.

Refs: TooTallNate/proxy-agents#79

* Partially revert 58ed8f0

Resuming the client socket is no longer needed as it is destroyed when
the target socket emits `'close'`.
@dependabot dependabot bot mentioned this pull request Mar 18, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lpinca @TooTallNate