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

Restify hangs when ending route chain without closing response #1774

Closed
3 tasks done
cprussin opened this issue Apr 12, 2019 · 2 comments
Closed
3 tasks done

Restify hangs when ending route chain without closing response #1774

cprussin opened this issue Apr 12, 2019 · 2 comments

Comments

@cprussin
Copy link
Contributor

  • 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

Bug Report

Restify Version

Tested on 8.3.0.

Node.js Version

Tested on v8.15.1.

Expected behaviour

Restify should not hang (in express this same case automatically 404s)

Actual behaviour

Restify hangs

Repro case

const restify = require('restify');
const server = restify.createServer();
server.get('/', (req, res, next) => next()); // also could be server.get('/', () => {});
server.listen(8080, () => console.log('Server up on port 8080'));

Cause

Restify does not detect cases where the middleware chain is completed and no response has been flushed.

Are you willing and able to fix this?

Yes

@retrohacker
Copy link
Member

Comparing this to express' behavior:

Express uses https://github.com/pillarjs/finalhandler at the end of the middleware chain. It does a check to see if headers have started being flushed yet as a heuristic.

This snippet 404s:

require('express')().get('/', function(req, res, next) { next(); }).listen(8080);

This snippet hangs:

require('express')().get('/', function(req, res, next) { res.flushHeaders(); next(); }).listen(8080);

It also requires calling the continuation, otherwise it hangs:

require('express')().get('/', function(req, res, next) {}).listen(8080);

@ghermeto
Copy link
Member

merged :)

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

3 participants