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

sendfile test fails with nodejs 18 #5014

Closed
hhorak opened this issue Oct 7, 2022 · 3 comments
Closed

sendfile test fails with nodejs 18 #5014

hhorak opened this issue Oct 7, 2022 · 3 comments
Labels

Comments

@hhorak
Copy link

hhorak commented Oct 7, 2022

I'm seeing this with RHEL build of nodejs 18, reported also in https://bugzilla.redhat.com/show_bug.cgi?id=2133093:

Version-Release number of selected component (if applicable):
nodejs-18.8.0-1.module_el8.7.0+1206+cd2bf569.x86_64

Steps to Reproduce:

  1. git clone https://github.com/expressjs/express.git
  2. cd express
  3. git checkout 4.18.1
  4. npm install
  5. npm test

Actual results:

<long output scratched>
    .sendfile(path, fn)
      ✔ should invoke the callback when complete
      ✔ should utilize the same options as express.static()
      ✔ should invoke the callback when client aborts
      ✔ should invoke the callback when client already aborted
      ✔ should invoke the callback without error when HEAD
      ✔ should invoke the callback without error when 304
      ✔ should invoke the callback on 404
      ✔ should not override manual content-types
      ✔ should invoke the callback on 403
      1) should invoke the callback on socket error


  1086 passing (3s)
  1 failing

  1) res
       .sendfile(path, fn)
         should invoke the callback on socket error:
     Uncaught Error: broken!
      at /opt/app-root/src/express/test/res.sendFile.js:1058:34
      at Layer.handle [as handle_request] (lib/router/layer.js:95:5)
      at trim_prefix (lib/router/index.js:328:13)
      at /opt/app-root/src/express/lib/router/index.js:286:9
      at Function.process_params (lib/router/index.js:346:12)
      at next (lib/router/index.js:280:10)
      at expressInit (lib/middleware/init.js:40:5)
      at Layer.handle [as handle_request] (lib/router/layer.js:95:5)
      at trim_prefix (lib/router/index.js:328:13)
      at /opt/app-root/src/express/lib/router/index.js:286:9
      at Function.process_params (lib/router/index.js:346:12)
      at next (lib/router/index.js:280:10)
      at query (lib/middleware/query.js:45:5)
      at Layer.handle [as handle_request] (lib/router/layer.js:95:5)
      at trim_prefix (lib/router/index.js:328:13)
      at /opt/app-root/src/express/lib/router/index.js:286:9
      at Function.process_params (lib/router/index.js:346:12)
      at next (lib/router/index.js:280:10)
      at Function.handle (lib/router/index.js:175:3)
      at Function.handle (lib/application.js:181:10)
      at Server.app (lib/express.js:39:9)
      at Server.emit (node:events:513:28)
      at parserOnIncoming (node:_http_server:1034:12)
      at HTTPParser.parserOnHeadersComplete (node:_http_common:117:17)
@dougwilson
Copy link
Contributor

Yea, something changes in Node.js 18.8, as it works in all Node.js 18 releases up to 18.7 (which is what our own CI tests), but not 18.8 onward. I saw it yesterday and haven't yet fully invested the cause.

@dougwilson
Copy link
Contributor

It looks like there was a change in behavior in Node.js 18.8 where the HTTP server no longer is always suppressing the socket errors when there are no other error listeners and instead allowing them to bubble up.

@hhorak
Copy link
Author

hhorak commented Oct 10, 2022

Thanks for a quick turn-around.

neohotsauce pushed a commit to neohotsauce/express that referenced this issue May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants