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

create-websocket-stream.test.js fails if the domain module is loaded #2124

Closed
1 task done
mvduin opened this issue Mar 9, 2023 · 2 comments · Fixed by #2126
Closed
1 task done

create-websocket-stream.test.js fails if the domain module is loaded #2124

mvduin opened this issue Mar 9, 2023 · 2 comments · Fixed by #2126

Comments

@mvduin
Copy link
Contributor

mvduin commented Mar 9, 2023

Is there an existing issue for this?

  • I've searched for any related issues and avoided creating a duplicate issue.

Description

If the domain module is loaded whentest/create-websocket-stream.test.js executes, it has some bizarre failures (see below). Other tests do not have this porblem.

I ran into this problem because I have NODE_OPTIONS="-r /home/matthijs/.node_defaults.js" in my environment to load a script that tweaks some REPL behaviour, for which I have to require repl which then requires domain.

While an argument could be made that it's impolite to have an environment that causes domain to be loaded into every node script, it's concerning for it to be causing test failures since that may imply actual bugs when ws is used in an environment that has domain loaded (e.g. whenever you're testing something from the REPL).

ws version

8.12.1

Node.js Version

14.21.3, 16.19.1, 18.14.2

System

No response

Expected result

No response

Actual result

No response

Attachments

Test output:

  createWebSocketStream
    ✓ is exposed as a property of the `WebSocket` class
    ✓ returns a `Duplex` stream
    ✓ passes the options object to the `Duplex` constructor
    The returned stream
      ✓ buffers writes if `readyState` is `CONNECTING`
      ✓ errors if a write occurs when `readyState` is `CLOSING`
      ✓ errors if a write occurs when `readyState` is `CLOSED`
      ✓ does not error if `_final()` is called while connecting
      ✓ makes `_final()` a noop if no socket is assigned
      ✓ reemits errors
      ✓ does not swallow errors that may occur while destroying
      1) does not suppress the throwing behavior of 'error' events
      2) is destroyed after 'end' and 'finish' are emitted (1/2)
      3) is destroyed after 'end' and 'finish' are emitted (1/2)
      ✓ is destroyed after 'end' and 'finish' are emitted (2/2)
      ✓ handles backpressure (1/3)
      ✓ handles backpressure (2/3)
      ✓ handles backpressure (3/3)
      ✓ can be destroyed (1/2)
      ✓ can be destroyed (2/2)
      ✓ converts text messages to strings in readable object mode
      ✓ resumes the socket if `readyState` is `CLOSING`


  18 passing (119ms)
  3 failing

  1) createWebSocketStream
       The returned stream
         does not suppress the throwing behavior of 'error' events:

      AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

2 !== 1

      + expected - actual

      -2
      +1
      
      at Context.<anonymous> (test/create-websocket-stream.test.js:298:14)
      at process.processImmediate (node:internal/timers:476:21)

  2) createWebSocketStream
       The returned stream
         is destroyed after 'end' and 'finish' are emitted (1/2):
     Uncaught RangeError: Invalid WebSocket frame: invalid opcode 5
      at Receiver.getInfo (lib/receiver.js:287:14)
      at Receiver.startLoop (lib/receiver.js:146:22)
      at Receiver._write (lib/receiver.js:84:10)
      at writeOrBuffer (node:internal/streams/writable:392:12)
      at _write (node:internal/streams/writable:333:10)
      at Writable.write (node:internal/streams/writable:337:10)
      at Socket.socketOnData (lib/websocket.js:1274:35)
      at Socket.emit (node:events:513:28)
      at Socket.emit (node:domain:489:12)
      at Readable.read (node:internal/streams/readable:539:10)
      at Socket.read (node:net:749:39)
      at flow (node:internal/streams/readable:1023:34)
      at emitReadable_ (node:internal/streams/readable:604:3)
      at process.processTicksAndRejections (node:internal/process/task_queues:81:21)

  3) createWebSocketStream
       The returned stream
         is destroyed after 'end' and 'finish' are emitted (1/2):
     Error: done() called multiple times in test <createWebSocketStream The returned stream is destroyed after 'end' and 'finish' are emitted (1/2)> of file /home/matthijs/dd/js/ws.tmp/test/create-websocket-stream.test.js
      at Object.onceWrapper (node:events:627:28)
      at WebSocketServer.emit (node:events:513:28)
      at WebSocketServer.emit (node:domain:489:12)
      at emitClose (lib/websocket-server.js:465:10)
      at Server.<anonymous> (lib/websocket-server.js:197:9)
      at Object.onceWrapper (node:events:627:28)
      at Server.emit (node:events:513:28)
      at Server.emit (node:domain:489:12)
      at emitCloseNT (node:net:2135:8)
      at process.processTicksAndRejections (node:internal/process/task_queues:81:21)
@mvduin
Copy link
Contributor Author

mvduin commented Mar 9, 2023

To reproduce you can put NODE_OPTIONS='-r domain' in your environment or run the test as node -r domain node_modules/.bin/mocha test/create-websocket-stream.test.js

@lpinca
Copy link
Member

lpinca commented Mar 9, 2023

This seems to fix the issue:

diff --git a/test/create-websocket-stream.test.js b/test/create-websocket-stream.test.js
index 4d51958..ab61ebe 100644
--- a/test/create-websocket-stream.test.js
+++ b/test/create-websocket-stream.test.js
@@ -285,7 +285,7 @@ describe('createWebSocketStream', () => {
       });
     });
 
-    it("does not suppress the throwing behavior of 'error' events", (done) => {
+    it.skip("does not suppress the throwing behavior of 'error' events", (done) => {
       const wss = new WebSocket.Server({ port: 0 }, () => {
         const ws = new WebSocket(`ws://localhost:${wss.address().port}`);
         createWebSocketStream(ws);

I did not investigate but I think it is related to the 'uncaughtException' event handling.

mvduin added a commit to dutchanddutch/node-ws that referenced this issue Mar 9, 2023
Fix a failure in test/create-websocket-stream.test.js if the node:domain
module is loaded (e.g. due to NODE_OPTIONS in environment).

The cause of the failure was that installing an 'uncaughtException'
event handler on process will cause node:domain to prepend its own
handler for the same event, which confused the test.

Fixes: websockets#2124
lpinca pushed a commit that referenced this issue Mar 9, 2023
Fix a failure in `test/create-websocket-stream.test.js` if the domain
module is loaded (e.g. due to `NODE_OPTIONS` in environment).

The cause of the failure was that installing an `'uncaughtException'`
event handler on `process` causes the domain module to prepend its own
handler for the same event, which confused the test.

Fixes #2124
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 a pull request may close this issue.

2 participants