GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — http: do not emit `upgrade` on advertisement by indutny · Pull Request #4337 · nodejs/node

Conversation

@indutny

@indutny indutny commented Dec 17, 2015

Copy link
Copy Markdown
Member

Do not emit upgrade if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

A server MAY send an Upgrade header field in any other response
to advertise that it implements support for upgrading to the
listed protocols, in order of descending preference, when
appropriate for a future request.

Fix: #4334

cc @nodejs/http
R?=@mscdex or @bnoordhuis

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@indutny
http: do not emit upgrade on advertisement
01fc2e9
Do not emit `upgrade` if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

    A server MAY send an Upgrade header field in any other response
    to advertise that it implements support for upgrading to the
    listed protocols, in order of descending preference, when
    appropriate for a future request.

Fix: nodejs#4334
@indutny indutny mentioned this pull request Dec 17, 2015
NodeJS disconnects when "Upgrade" HTTP header is used for example to advertise HTTP/2 support #4334
Closed
@indutny

indutny commented Dec 17, 2015

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/1021/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fishrock123 Fishrock123 added the http Issues and PRs related to the http subsystem. label Dec 17, 2015
@Fishrock123

Fishrock123 commented Dec 17, 2015

Copy link
Copy Markdown
Contributor

This probably needs to be backported to 0.10 and 0.12 if possible.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MylesBorins MylesBorins added lts-watch-v4.x labels Dec 17, 2015
@indutny

indutny commented Dec 17, 2015

Copy link
Copy Markdown
Member Author

@Fishrock123 we should we very careful with this. As you have probably already seen in the tests - there is some behavior change, and we should be sure that this is necessary.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell

jasnell commented Dec 17, 2015

Copy link
Copy Markdown
Member

Hmm.. yeah we'll want to test that out. Use of Upgrade has been rare up until HTTP/2 so we're likely safe, but there could be some folks out there making undocumented and unknown uses that could be affected.

One question.. is this enough of a change of behavior to warrant a semver-minor or major? The old behavior may have been against spec, but it technically was working as written -- i.e. it wasn't a bug.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@indutny

indutny commented Dec 17, 2015

Copy link
Copy Markdown
Member Author

@jasnell I wouldn't do semver-minor for this, either patch or major. If we will decide to not backport it - let's call it major.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell

jasnell commented Dec 17, 2015

Copy link
Copy Markdown
Member

As much as I'd prefer to have this in v4 and earlier, I'm not sure I'm comfortable with doing so... currently leaning towards major then.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dougwilson

dougwilson commented Dec 17, 2015

Copy link
Copy Markdown
Member

As much as I'd prefer to have this in v4 and earlier, I'm not sure I'm comfortable with doing so... currently leaning towards major then.

I second that.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@indutny indutny added semver-major PRs that contain breaking changes and should be released in the next major version. and removed lts-watch-v0.10 labels Dec 18, 2015
@indutny

indutny commented Dec 18, 2015

Copy link
Copy Markdown
Member Author

Let it be it then!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnoordhuis
bnoordhuis reviewed Dec 18, 2015
View reviewed changes
Comment thread lib/_http_common.js Outdated

@bnoordhuis bnoordhuis Dec 18, 2015

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment explaining that the regex is supposed to match Connection: headers containing words besides just the string 'upgrade'?

(At least, that's what I infer it does.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@indutny
add comment
f64bf32
bnoordhuis
bnoordhuis reviewed Dec 18, 2015
View reviewed changes
Comment thread test/parallel/test-http-upgrade-advertise.js Outdated

@bnoordhuis bnoordhuis Dec 18, 2015

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: you could just pass fire() directly.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bnoordhuis

bnoordhuis commented Dec 18, 2015

Copy link
Copy Markdown
Member

LGTM with two unimportant nits.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@indutny
test
a48ed32
@indutny

indutny commented Dec 18, 2015

Copy link
Copy Markdown
Member Author

@bnoordhuis thank you!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@indutny

indutny commented Dec 18, 2015

Copy link
Copy Markdown
Member Author

Landed in 32ac376, thank you!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

indutny added a commit that referenced this pull request Dec 18, 2015
@indutny
http: do not emit upgrade on advertisement
32ac376
Do not emit `upgrade` if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

    A server MAY send an Upgrade header field in any other response
    to advertise that it implements support for upgrading to the
    listed protocols, in order of descending preference, when
    appropriate for a future request.

Fix: #4334
PR-URL: #4337
Reviewed-By: Ben Noordhuis <[email protected]>
@mscdex mscdex closed this Dec 18, 2015
@umutm

umutm commented Jan 20, 2016

Copy link
Copy Markdown

When can we expect this feature to be added to the core?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Trott

Trott commented Jan 20, 2016

Copy link
Copy Markdown
Member

@umutm It's tagged semver-major so it won't be available in a release until Node 6 which will be out in April. (See the graphic at the bottom of https://github.com/nodejs/LTS/.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MylesBorins MylesBorins mentioned this pull request Feb 17, 2016
http: remove reference to onParserExecute #4773
Closed
@rvagg rvagg mentioned this pull request Feb 18, 2016
Release proposal: 5.7.0 (Stable) #5295
Merged
rvagg added a commit that referenced this pull request Feb 21, 2016
@rvagg
2016-02-23, Version 5.7.0 (Stable)
3cc524e
* buffer:
  - You can now supply an encoding argument when filling a
    Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying
    an existing Buffer will also work with
    Buffer#fill(buffer[, start[, end]]). See the API documentation for
    details on how this works. (Trevor Norris) #4935
  - Buffer#indexOf() no longer requires a byteOffset argument if you
    also wish to specify an encoding:
    Buffer#indexOf(val[, byteOffset][, encoding]).
    (Trevor Norris) #4803
* child_process: spawn() and spawnSync() now support a 'shell' option
  to allow for optional execution of the given command inside a shell.
  If set to true, cmd.exe will be used on Windows and /bin/sh
  elsewhere. A path to a custom shell can also be passed to override
  these defaults. On Windows, this option allows .bat. and .cmd files
  to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598
* http_parser: Update to http-parser 2.6.2 to fix an unintentionally
  strict limitation of allowable header characters.
  (James M Snell) #5237
* dgram: socket.send() now supports accepts an array of Buffers or
  Strings as the first argument. See the API docs for details on how
  this works. (Matteo Collina) #4374
* http: Fix a bug where handling headers will mistakenly trigger an
  'upgrade' event where the server is just advertising its protocols.
  This bug can prevent HTTP clients from communicating with HTTP/2
  enabled servers. (Fedor Indutny) #4337
* net: Added a listening Boolean property to net and http servers to
  indicate whether the server is listening for connections.
  (José Moreira) #4743
* node: The C++ node::MakeCallback() API is now reentrant and calling
  it from inside another MakeCallback() call no longer causes the
  nextTick queue or Promises microtask queue to be processed out of
  order. (Trevor Norris) #4507
* tls: Add a new tlsSocket.getProtocol() method to get the negotiated
  TLS protocol version of the current connection. (Brian White) #4995
* vm: Introduce new 'produceCachedData' and 'cachedData' options to
  new vm.Script() to interact with V8's code cache. When a new
  vm.Script object is created with the 'produceCachedData' set to true
  a Buffer with V8's code cache data will be produced and stored in
  cachedData property of the returned object. This data in turn may be
  supplied back to another vm.Script() object with a 'cachedData'
  option if the supplied source is the same. Successfully executing a
  script from cached data can speed up instantiation time. See the API
  docs for details. (Fedor Indutny) #4777
* performance: Improvements in:
  - process.nextTick() (Ruben Bridgewater) #5092
  - path module (Brian White) #5123
  - querystring module (Brian White) #5012
  - streams module when processing small chunks (Matteo Collina) #4354
rvagg added a commit that referenced this pull request Feb 21, 2016
@rvagg
2016-02-23, Version 5.7.0 (Stable)
f02e0c0
* buffer:
  - You can now supply an encoding argument when filling a
    Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying
    an existing Buffer will also work with
    Buffer#fill(buffer[, start[, end]]). See the API documentation for
    details on how this works. (Trevor Norris) #4935
  - Buffer#indexOf() no longer requires a byteOffset argument if you
    also wish to specify an encoding:
    Buffer#indexOf(val[, byteOffset][, encoding]).
    (Trevor Norris) #4803
* child_process: spawn() and spawnSync() now support a 'shell' option
  to allow for optional execution of the given command inside a shell.
  If set to true, cmd.exe will be used on Windows and /bin/sh
  elsewhere. A path to a custom shell can also be passed to override
  these defaults. On Windows, this option allows .bat. and .cmd files
  to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598
* http_parser: Update to http-parser 2.6.2 to fix an unintentionally
  strict limitation of allowable header characters.
  (James M Snell) #5237
* dgram: socket.send() now supports accepts an array of Buffers or
  Strings as the first argument. See the API docs for details on how
  this works. (Matteo Collina) #4374
* http: Fix a bug where handling headers will mistakenly trigger an
  'upgrade' event where the server is just advertising its protocols.
  This bug can prevent HTTP clients from communicating with HTTP/2
  enabled servers. (Fedor Indutny) #4337
* net: Added a listening Boolean property to net and http servers to
  indicate whether the server is listening for connections.
  (José Moreira) #4743
* node: The C++ node::MakeCallback() API is now reentrant and calling
  it from inside another MakeCallback() call no longer causes the
  nextTick queue or Promises microtask queue to be processed out of
  order. (Trevor Norris) #4507
* tls: Add a new tlsSocket.getProtocol() method to get the negotiated
  TLS protocol version of the current connection. (Brian White) #4995
* vm: Introduce new 'produceCachedData' and 'cachedData' options to
  new vm.Script() to interact with V8's code cache. When a new
  vm.Script object is created with the 'produceCachedData' set to true
  a Buffer with V8's code cache data will be produced and stored in
  cachedData property of the returned object. This data in turn may be
  supplied back to another vm.Script() object with a 'cachedData'
  option if the supplied source is the same. Successfully executing a
  script from cached data can speed up instantiation time. See the API
  docs for details. (Fedor Indutny) #4777
* performance: Improvements in:
  - process.nextTick() (Ruben Bridgewater) #5092
  - path module (Brian White) #5123
  - querystring module (Brian White) #5012
  - streams module when processing small chunks (Matteo Collina) #4354
rvagg added a commit that referenced this pull request Feb 23, 2016
@rvagg
2016-02-23, Version 5.7.0 (Stable)
cf317e2
* buffer:
  - You can now supply an encoding argument when filling a
    Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying
    an existing Buffer will also work with
    Buffer#fill(buffer[, start[, end]]). See the API documentation for
    details on how this works. (Trevor Norris) #4935
  - Buffer#indexOf() no longer requires a byteOffset argument if you
    also wish to specify an encoding:
    Buffer#indexOf(val[, byteOffset][, encoding]).
    (Trevor Norris) #4803
* child_process: spawn() and spawnSync() now support a 'shell' option
  to allow for optional execution of the given command inside a shell.
  If set to true, cmd.exe will be used on Windows and /bin/sh
  elsewhere. A path to a custom shell can also be passed to override
  these defaults. On Windows, this option allows .bat. and .cmd files
  to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598
* http_parser: Update to http-parser 2.6.2 to fix an unintentionally
  strict limitation of allowable header characters.
  (James M Snell) #5237
* dgram: socket.send() now supports accepts an array of Buffers or
  Strings as the first argument. See the API docs for details on how
  this works. (Matteo Collina) #4374
* http: Fix a bug where handling headers will mistakenly trigger an
  'upgrade' event where the server is just advertising its protocols.
  This bug can prevent HTTP clients from communicating with HTTP/2
  enabled servers. (Fedor Indutny) #4337
* net: Added a listening Boolean property to net and http servers to
  indicate whether the server is listening for connections.
  (José Moreira) #4743
* node: The C++ node::MakeCallback() API is now reentrant and calling
  it from inside another MakeCallback() call no longer causes the
  nextTick queue or Promises microtask queue to be processed out of
  order. (Trevor Norris) #4507
* tls: Add a new tlsSocket.getProtocol() method to get the negotiated
  TLS protocol version of the current connection. (Brian White) #4995
* vm: Introduce new 'produceCachedData' and 'cachedData' options to
  new vm.Script() to interact with V8's code cache. When a new
  vm.Script object is created with the 'produceCachedData' set to true
  a Buffer with V8's code cache data will be produced and stored in
  cachedData property of the returned object. This data in turn may be
  supplied back to another vm.Script() object with a 'cachedData'
  option if the supplied source is the same. Successfully executing a
  script from cached data can speed up instantiation time. See the API
  docs for details. (Fedor Indutny) #4777
* performance: Improvements in:
  - process.nextTick() (Ruben Bridgewater) #5092
  - path module (Brian White) #5123
  - querystring module (Brian White) #5012
  - streams module when processing small chunks (Matteo Collina) #4354

PR-URL: #5295
rvagg added a commit that referenced this pull request Feb 24, 2016
@rvagg
2016-02-23, Version 5.7.0 (Stable)
7b0a83d
* buffer:
  - You can now supply an encoding argument when filling a
    Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying
    an existing Buffer will also work with
    Buffer#fill(buffer[, start[, end]]). See the API documentation for
    details on how this works. (Trevor Norris) #4935
  - Buffer#indexOf() no longer requires a byteOffset argument if you
    also wish to specify an encoding:
    Buffer#indexOf(val[, byteOffset][, encoding]).
    (Trevor Norris) #4803
* child_process: spawn() and spawnSync() now support a 'shell' option
  to allow for optional execution of the given command inside a shell.
  If set to true, cmd.exe will be used on Windows and /bin/sh
  elsewhere. A path to a custom shell can also be passed to override
  these defaults. On Windows, this option allows .bat. and .cmd files
  to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598
* http_parser: Update to http-parser 2.6.2 to fix an unintentionally
  strict limitation of allowable header characters.
  (James M Snell) #5237
* dgram: socket.send() now supports accepts an array of Buffers or
  Strings as the first argument. See the API docs for details on how
  this works. (Matteo Collina) #4374
* http: Fix a bug where handling headers will mistakenly trigger an
  'upgrade' event where the server is just advertising its protocols.
  This bug can prevent HTTP clients from communicating with HTTP/2
  enabled servers. (Fedor Indutny) #4337
* net: Added a listening Boolean property to net and http servers to
  indicate whether the server is listening for connections.
  (José Moreira) #4743
* node: The C++ node::MakeCallback() API is now reentrant and calling
  it from inside another MakeCallback() call no longer causes the
  nextTick queue or Promises microtask queue to be processed out of
  order. (Trevor Norris) #4507
* tls: Add a new tlsSocket.getProtocol() method to get the negotiated
  TLS protocol version of the current connection. (Brian White) #4995
* vm: Introduce new 'produceCachedData' and 'cachedData' options to
  new vm.Script() to interact with V8's code cache. When a new
  vm.Script object is created with the 'produceCachedData' set to true
  a Buffer with V8's code cache data will be produced and stored in
  cachedData property of the returned object. This data in turn may be
  supplied back to another vm.Script() object with a 'cachedData'
  option if the supplied source is the same. Successfully executing a
  script from cached data can speed up instantiation time. See the API
  docs for details. (Fedor Indutny) #4777
* performance: Improvements in:
  - process.nextTick() (Ruben Bridgewater) #5092
  - path module (Brian White) #5123
  - querystring module (Brian White) #5012
  - streams module when processing small chunks (Matteo Collina) #4354

PR-URL: #5295
@MylesBorins MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 1, 2016
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Mar 1, 2016
@indutny Myles Borins
http: do not emit upgrade on advertisement
92ec8a9
Do not emit `upgrade` if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

    A server MAY send an Upgrade header field in any other response
    to advertise that it implements support for upgrading to the
    listed protocols, in order of descending preference, when
    appropriate for a future request.

Fix: nodejs#4334
PR-URL: nodejs#4337
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
@indutny Myles Borins
http: do not emit upgrade on advertisement
6a0571c
Do not emit `upgrade` if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

    A server MAY send an Upgrade header field in any other response
    to advertise that it implements support for upgrading to the
    listed protocols, in order of descending preference, when
    appropriate for a future request.

Fix: #4334
PR-URL: #4337
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
@indutny Myles Borins
http: do not emit upgrade on advertisement
def3bb2
Do not emit `upgrade` if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

    A server MAY send an Upgrade header field in any other response
    to advertise that it implements support for upgrading to the
    listed protocols, in order of descending preference, when
    appropriate for a future request.

Fix: #4334
PR-URL: #4337
Reviewed-By: Ben Noordhuis <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@indutny Michael Scovetta
http: do not emit upgrade on advertisement
d59d66f
Do not emit `upgrade` if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

    A server MAY send an Upgrade header field in any other response
    to advertise that it implements support for upgrading to the
    listed protocols, in order of descending preference, when
    appropriate for a future request.

Fix: nodejs#4334
PR-URL: nodejs#4337
Reviewed-By: Ben Noordhuis <[email protected]>
@umutm

umutm commented Apr 10, 2016

Copy link
Copy Markdown

Any chance that this fix will be added to v0.10 to make the older Nodejsversions compatible with HTTP/2?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bnoordhuis

bnoordhuis commented Apr 11, 2016

Copy link
Copy Markdown
Member

@umutm I don't know if any collaborators are interested in back-porting it but you're welcome to give it a go; the v0.12-staging and v0.10-staging branches are what you want to target.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@umutm

umutm commented Apr 11, 2016

Copy link
Copy Markdown

@bnoordhuis Thanks very much for the update.

Although I'd love to, that is over my talent and had wondered if there was any official decision to make older versions HTTP\2 compatible. Thanks very much again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MylesBorins

MylesBorins commented Apr 11, 2016

Copy link
Copy Markdown
Contributor

afaik 0.10 and 0.12 are not going to be getting any non security upgrades.

/cc @nodejs/lts

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rvagg

rvagg commented Apr 11, 2016

Copy link
Copy Markdown
Member

0.12 entered Maintenance this month and joins 0.10, the bar is pretty high for getting changes in to those branches however given the importance of this change to supporting modern servers I think we might be at least open to have a discussion about if if someone were to put pull requests to those staging branches.

Your best bet is to migrate to v4+ tbh, there's so much additional work that's gone in to making v4 a really solid platform.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@indutny

indutny commented Apr 11, 2016

Copy link
Copy Markdown
Member Author

Please also note that this is a breaking change, some programs that work right now on v0.10/v0.12 may become broken after backporting it. Can we afford this?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@umutm

umutm commented Apr 11, 2016

Copy link
Copy Markdown

Also, strange enough, there is not much demand for this. It is either "big hosting companies didn't update their Apache versions yet so that HTTP requests work fine for many" or the majority uses v4+.

We have performed a switch to v4, experiencing an issue there (couldn't clarify yet) and wanted to double check if the v0.1+ would get this feature. An totally understand the difficulty of maintaining/updating such older versions.

Thanks you very much again for all the help.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fishrock123

Fishrock123 commented Apr 11, 2016

Copy link
Copy Markdown
Contributor

wanted to double check if the v0.1+ would get this feature.

@umutm Probably not very immediately at the least, if at all.

If you need help upgrading to v4 we have a full list of breaking changes v0.10 -> v4.
There are also tools such as upgrade-ready that can help find outdated modules.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tunetheweb tunetheweb mentioned this pull request Apr 12, 2016
NodeJS unable to make https requests when h2 enabled in Apache 2.4.18 icing/mod_h2#73
Closed
@voxsoftware voxsoftware mentioned this pull request Jul 6, 2016
Please include this fix in JxCore https://github.com/nodejs/node/pull/4337 jxcore/jxcore#895
Open
@iamjochem iamjochem mentioned this pull request Mar 21, 2017
Servers that respond with "Upgrade: h2c" makes 'the world end' (Node v4) request/request#2601
Closed
@SimenB SimenB mentioned this pull request Aug 6, 2026
fix(jest-haste-map): tolerate transient EPERM in the watcher lstat jestjs/jest#16295
Merged
@SimenB SimenB mentioned this pull request Aug 17, 2026
fix(jest-haste-map): tolerate a locked file while indexing on Windows jestjs/jest#16358
Merged
SimenB added a commit to SimenB/jest that referenced this pull request Aug 17, 2026
@SimenB @claude
test(jest-haste-map): pin workerError to the ignorable-error predicate
a618f0f
The added tests covered `EACCES`, `ENOENT` and `EISDIR`, all of which behave
identically before and after the change, so reverting the fix outright left
them green — the `EPERM`-on-Windows path they exist for was never asserted.
Cover both platforms, loading `FileProcessor` through `jest.isolateModules`
because the predicate reads the platform once at module load.

Also fix two claims found in review. The comment cited `nodejs/node#4337`,
which is an HTTP Upgrade change rather than anything about file locking; drop
the reference and describe the behaviour on its own. And `statCache` spares
repeated `lstat` calls only best-effort — the per-root walks run concurrently
and can both miss a path before either caches it, as `walk.ts` already notes.

Co-Authored-By: Claude Fable 5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

http Issues and PRs related to the http subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

13 participants

@indutny @Fishrock123 @jasnell @dougwilson @bnoordhuis @umutm @Trott @vkurchatkin @silverwind @mscdex @djc @rvagg @MylesBorins
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.