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

Ignore node network errors after opening client socket #220

Merged
merged 2 commits into from Mar 7, 2018

Conversation

@mtharrison
Copy link
Member

mtharrison commented Mar 6, 2018

This is another stab at fixing High Sierra tests for nes.

Since ws 3.3.3, node net.Socket errors are reemitted by ws. We aren't really interested in these events, since they are not emitted by browser implementations of websockets. What we're interested in in those cases is the close event which will be emitted by both browser and ws.

In our tests we close both server and websocket at the same time, meaning the server could close the TCP socket at the same time the client sends a close frame, which causes a ECONNRESET socket error. For some reason (timing difference?) this manifests in Mac OS X High Sierra by logging or throwing an error for each test. In some cases it just causes console noise. in other cases where no onerror is defined it will throw and cause the test to fail.

I brought this up on ws: websockets/ws#1316

The advice is to ignore net.Socket errors once the websocket is open, and in fact these won't even be emitted by ws in future versions. Doing so in nes and attaching onerror handlers for where we use raw ws in the tests fixes all the test issues AFAIK.

Closes #216

@mtharrison mtharrison added the bug label Mar 6, 2018
@mtharrison

This comment has been minimized.

Copy link
Member Author

mtharrison commented Mar 6, 2018

Guess I need a test for all the platforms that aren't behaving like Mac...

@@ -479,6 +479,7 @@ describe('Socket', () => {

await server.start();
const client = new Ws('http://localhost:' + server.info.port);
client.onerror = Hoek.ignore;

This comment has been minimized.

Copy link
@devinivy

devinivy Mar 6, 2018

Member

Are these still necessary?

This comment has been minimized.

Copy link
@mtharrison

mtharrison Mar 6, 2018

Author Member

Yes, if there's no error handler attached it will throw a read ECONNRESET error.

Copy link
Member

devinivy left a comment

👍 👌

@mtharrison mtharrison merged commit 4b5c89a into master Mar 7, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@mtharrison mtharrison deleted the fix-hs2 branch Mar 7, 2018
@mtharrison mtharrison added this to the 7.0.4 milestone Mar 7, 2018
@mtharrison mtharrison self-assigned this Mar 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.