restify / restify/node-restify

Error during connection upgrade leads to uncaught write-after-end error

Open
#1,857 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
JavaScript
Stars
10.7k
Forks
975
Avg merge
1d 6h
Merged PRs (30d)
5

Description

  • Used appropriate template for the issue type
  • Searched both open and closed issues for duplicates of this issue
  • Title adequately and concisely reflects the feature or the bug

Restify Version: 8.5.1
Node.js Version: v10.15.2

Expected behaviour

Error handling on web socket routes should not lead to uncaught errors.

Actual behaviour

An error during the connection upgrade results in a write-after-end error.

Workaround

Suppressing the thrown error is possible, if inelegant:

server.on('restifyError', (req, res, err, done) => {
  try { done(); } catch(e) { /* suppress the error */ }
})

Cause

The cause appears to be that for regular (non-upgrade) requests, the_emitErrorEvents() callback in server.js has a guard against double handling by checking res._sent. For upgrade responses, this flag does not exist. Rather, it appears to use res._headWritten. Modifying _writeHeadImpl() in upgrade.js to also set this._sent in addition to this._headWritten avoids the write-after-end error. Alternatively, changing the emitError() function to also check res._headWritten should have the same effect.

Are you willing and able to fix this?

See above for proposed fix. I'm not familiar enough with the project internals to want to PR myself.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in server.js at the _emitErrorEvents() callback and compare its res._sent guard with upgrade.js, especially _writeHeadImpl() and the emitError() path. Reproduce the connection-upgrade error and verify that websocket-route error handling no longer produces an uncaught write-after-end error.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js
Domain
api, backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.