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

Does ws work with express-status-monitor #1664

Closed
1 task done
yossisp opened this issue Nov 21, 2019 · 5 comments
Closed
1 task done

Does ws work with express-status-monitor #1664

yossisp opened this issue Nov 21, 2019 · 5 comments

Comments

@yossisp
Copy link

yossisp commented Nov 21, 2019

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

Description

When using ws with express and adding express-status-monitor npm package as middleware, the server crashes with the following error:

node_modules/ws/lib/websocket.js:837
  websocket.readyState = WebSocket.CLOSING;
                       ^

TypeError: Cannot set property 'readyState' of undefined
    at Socket.socketOnClose (node_modules/ws/lib/websocket.js:837:24)
    at Socket.emit (events.js:194:15)
    at Socket.EventEmitter.emit (domain.js:441:20)
    at TCP._handle.close (net.js:600:12)
npm ERR! code ELIFECYCLE

Reproducible in:

  • version: ^7.2.0
  • Node.js version(s): 10.15.1
  • OS version(s): macOS mojave

Steps to reproduce:

import statusMonitor from 'express-status-monitor'
import debug from 'debug'
import express from 'express'
import WebSocket from 'isomorphic-ws'
import { createServer } from 'http'

const app = express()
app.use(statusMonitor())
  const server = createServer(app)
  const wsServer = new WebSocket.Server({
    server,
    perMessageDeflate: false,
    clientTracking: true
  })

  wsServer.on(events.connection, function (wsClient) {})
server.listen(constants.server.port, function () {
    log(`Express server listening on http://localhost:${constants.server.port}`)
  })

Expected result:

server doesn't crash

Actual result:

server crashes

Attachments:

@lpinca
Copy link
Member

lpinca commented Nov 21, 2019

express-status-monitor uses socket.io which uses ws, so what probably happens here is that two ws servers are created that share the same HTTP server. Two 'upgrade' listeners are added so two 'close' listeners are added to the same underlying net.Socket. As a result socketOnClose() is called twice leading to this error.

The underlying issue is the same of #1660. It's invalid usage. There must be a 1:1 correspondence between a WebSocket and a net.Socket.

@yossisp
Copy link
Author

yossisp commented Nov 21, 2019

Thank you for the quick response! Do you know any monitoring tools that could be used with ws?

@lpinca
Copy link
Member

lpinca commented Nov 21, 2019

No. it depends on the project but a proper APM like Elastic APM https://www.elastic.co/guide/en/apm/agent/nodejs/current/express.html might be better.

In the example above is there any reason to create your own WebSocket server? As far as I can see, it is possible to create a socket.io server externally and pass it to express-status-monitor so you could tell socket.io to use only websocket for example but yes this is not the same of using ws.

@yossisp
Copy link
Author

yossisp commented Nov 21, 2019

I need to monitor the health of websocket server so it's handy to attach health route to that server in order to monitor it and restart if needed. That's why I need websocket to use express server.

@lpinca
Copy link
Member

lpinca commented Nov 22, 2019

Closing as answered. Discussion can continue if needed.

@lpinca lpinca closed this as completed Nov 22, 2019
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

No branches or pull requests

2 participants