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

POSTs whose bodys are instances of createReadStream hang with latest node #2341

Open
2 tasks done
stephen-willoughby opened this issue Mar 26, 2024 · 5 comments
Open
2 tasks done

Comments

@stephen-willoughby
Copy link

stephen-willoughby commented Mar 26, 2024

Describe the bug

  • Node.js version: 20.11.1
  • OS & version: macos 14.4

Whilst trying to move to support node 20, I have come across the following behaviour. If I drop to node 20.9.0 it works correctly. If I replace got with axios it works correctly.

Of note is nock/nock#2595 but as switching to axios seems to work and everyone on the nock issue is using got, got seems the more likely candidate.

Actual behavior

this code completes with 13 and nothing outputted
...

Expected behavior

I should outputted the body { foo: 'bar' } and complete
...

Code to reproduce

Note you need to provide somefile.txt, its content is irrelevant.

import nock from 'nock'
import { createReadStream } from 'fs'
import got from 'got'

const myFile = 'somefile.txt'
const host = 'http://example.com'
const path = '/some/path'
const response = { foo: 'bar' }
const options = {
  method: 'post',
  url: host + path,
  body: createReadStream(myFile)
}

nock(host)
  .post(path)
  .reply(200, response)

try {
  const result = await got(options)
  console.log(result.body)
} catch (error) {
  console.error('error')
  console.error(error)
}
console.log('complete')

Checklist

  • I have read the documentation.
  • I have tried my code with the latest version of Node.js and Got.
@stephen-willoughby
Copy link
Author

For old node

{"foo":"bar"}
complete

Process finished with exit code 0

for node 20.11.1

Process finished with exit code 13

@sindresorhus
Copy link
Owner

Does it work without Nock? If yes, then the issue is definitely Nock as it should work with anything that normally works without it.

You can replace Nock with https://httpbin.org for testing.

@stephen-willoughby
Copy link
Author

Yes, it works without nock, replacing it with https://httpbin.org/post, but similarly nock works with axios in place of got.

@masontikhonov
Copy link

@sindresorhus I tend to agree that it might be an issue with Got, in particular with asPromise(request), which returns promise that never resolves in some rare conditions (changes to streams that were done in node v20.10.0 + some nock behavior).

@aduh95
Copy link

aduh95 commented Apr 17, 2024

FYI patching Nock source the following way resolved the issue for me:

index d82c2446b540759cbc54ad7b26222a80109b8ec9..bf24add845d12e436c97b3ea5e2e38f87b77d489 100644
--- a/lib/intercepted_request_router.js
+++ b/lib/intercepted_request_router.js
@@ -68,7 +68,7 @@ class InterceptedRequestRouter {
     // For parity with Node, it's important the socket event is emitted before we begin playback.
     // This flag is set when playback is triggered if we haven't yet gotten the
     // socket event to indicate that playback should start as soon as it comes in.
-    this.readyToStartPlaybackOnSocketEvent = false
+    this.readyToStartPlaybackOnSocketEvent = true

     this.attachToReq()

Jkovarik added a commit to nasa/cumulus that referenced this issue Apr 23, 2024
This update has a couple of controversial changes in it:

Updating got to v14 means we're using a pure ESM module given sindre's
stance on not supporting common exports.  That's being done due to
incompatible changes in node streams requires at least got v12

Additionally there's a probable outstanding issue in got
sindresorhus/got#2341 related to node v20/fs
readstreams/nock and/or msw incompatibility (as well as a possible
open source contrib)
Jkovarik added a commit to nasa/cumulus that referenced this issue Apr 30, 2024
* Update core deps to v20.12.2

* Update unit test to rmove .sort

.sort is intermittently failing to order as expected, causing this
test to fail in local test at an alarming failure rate.    Updating
test to be more explicit/remove sort, however this needs to be
investigated prior to closing thie branch/PR/ticket

* Update kinesis test to node v20

JSON.parse now throws a different error for the merged test case.

* Update knex test to handle new subdependency throwing an Aggregate
error

* Revert "Update unit test to rmove .sort"

This reverts commit adbad84.

* Update got

* Revert "Update got"

This reverts commit a19f9d1.

* Upgrade ava/nock, fix ingest provider tests

* Reapply "Update got"

This reverts commit e652516.

* Update ingest module for node 20

This update has a couple of controversial changes in it:

Updating got to v14 means we're using a pure ESM module given sindre's
stance on not supporting common exports.  That's being done due to
incompatible changes in node streams requires at least got v12

Additionally there's a probable outstanding issue in got
sindresorhus/got#2341 related to node v20/fs
readstreams/nock and/or msw incompatibility (as well as a possible
open source contrib)

* Remove httpClient test mock/fix

Updating the existing apache server to return 200 on an existing
endpoint is far better than the prior commit approach in that it's a
valid/useful unit test as a result, with the technical/less tidy
downside of requiring the unit test stack to be working.

* Update local test stack configuration to restrict connection to localhost

* Update send-pan test/dependencies

* Update lambdas/async operation to deploy with node 20

* Update common to export a helper to import ESM got module

* Add docstring to importGot

* Remove unneeded imports

* Minor PR review fixes

* Update CHANGELOG

* Update CHANGELOG

* Reconfigure CI to not use unsafe perms modification

* Fix broken package.json

* Fix extran. dep issue

* Make error message test less specific

* Update test to not rely on aggregate internal errors

* Update packages/ingest/test/test-HttpProviderClient.js

Co-authored-by: etcart <37375117+etcart@users.noreply.github.com>

* Move importGot -> importEsm, introduce static import definition

* Fix inadvertant test commit

* Update CMA-js dependency to v2.2.0

* Remove dynamic/frozen import

---------

Co-authored-by: etcart <37375117+etcart@users.noreply.github.com>
Jkovarik added a commit to nasa/cumulus that referenced this issue May 6, 2024
* Update core deps to v20.12.2

* Update unit test to rmove .sort

.sort is intermittently failing to order as expected, causing this
test to fail in local test at an alarming failure rate.    Updating
test to be more explicit/remove sort, however this needs to be
investigated prior to closing thie branch/PR/ticket

* Update kinesis test to node v20

JSON.parse now throws a different error for the merged test case.

* Update knex test to handle new subdependency throwing an Aggregate
error

* Revert "Update unit test to rmove .sort"

This reverts commit adbad84.

* Update got

* Revert "Update got"

This reverts commit a19f9d1.

* Upgrade ava/nock, fix ingest provider tests

* Reapply "Update got"

This reverts commit e652516.

* Update ingest module for node 20

This update has a couple of controversial changes in it:

Updating got to v14 means we're using a pure ESM module given sindre's
stance on not supporting common exports.  That's being done due to
incompatible changes in node streams requires at least got v12

Additionally there's a probable outstanding issue in got
sindresorhus/got#2341 related to node v20/fs
readstreams/nock and/or msw incompatibility (as well as a possible
open source contrib)

* Remove httpClient test mock/fix

Updating the existing apache server to return 200 on an existing
endpoint is far better than the prior commit approach in that it's a
valid/useful unit test as a result, with the technical/less tidy
downside of requiring the unit test stack to be working.

* Update local test stack configuration to restrict connection to localhost

* Update send-pan test/dependencies

* Update lambdas/async operation to deploy with node 20

* Update common to export a helper to import ESM got module

* Add docstring to importGot

* Remove unneeded imports

* Minor PR review fixes

* Update CHANGELOG

* Update CHANGELOG

* Reconfigure CI to not use unsafe perms modification

* Fix broken package.json

* Fix extran. dep issue

* Make error message test less specific

* Update test to not rely on aggregate internal errors

* Update packages/ingest/test/test-HttpProviderClient.js

Co-authored-by: etcart <37375117+etcart@users.noreply.github.com>

* Move importGot -> importEsm, introduce static import definition

* Fix inadvertant test commit

* Update rules helpers to remove all null key values from rule JSON

* Update CHANGELOG

* Update helper test titles for consistency

* Minor refactor/rename

* Fix typing errors in original code, update PR accordingly

* Fix inadvertant move of rule validation

* Update packages/api/endpoints/rules.js

Co-authored-by: etcart <37375117+etcart@users.noreply.github.com>

* Remove custom null removal method in favor of omitDeepBy

* Remove duplicative call to omitDeepBy

* Update packages/api/lib/rulesHelpers.js

Co-authored-by: etcart <37375117+etcart@users.noreply.github.com>

---------

Co-authored-by: etcart <37375117+etcart@users.noreply.github.com>
Jkovarik added a commit to nasa/cumulus that referenced this issue May 6, 2024
* Update core deps to v20.12.2

* Update unit test to rmove .sort

.sort is intermittently failing to order as expected, causing this
test to fail in local test at an alarming failure rate.    Updating
test to be more explicit/remove sort, however this needs to be
investigated prior to closing thie branch/PR/ticket

* Update kinesis test to node v20

JSON.parse now throws a different error for the merged test case.

* Update knex test to handle new subdependency throwing an Aggregate
error

* Revert "Update unit test to rmove .sort"

This reverts commit adbad84.

* Update got

* Revert "Update got"

This reverts commit a19f9d1.

* Upgrade ava/nock, fix ingest provider tests

* Reapply "Update got"

This reverts commit e652516.

* Update ingest module for node 20

This update has a couple of controversial changes in it:

Updating got to v14 means we're using a pure ESM module given sindre's
stance on not supporting common exports.  That's being done due to
incompatible changes in node streams requires at least got v12

Additionally there's a probable outstanding issue in got
sindresorhus/got#2341 related to node v20/fs
readstreams/nock and/or msw incompatibility (as well as a possible
open source contrib)

* Remove httpClient test mock/fix

Updating the existing apache server to return 200 on an existing
endpoint is far better than the prior commit approach in that it's a
valid/useful unit test as a result, with the technical/less tidy
downside of requiring the unit test stack to be working.

* Update local test stack configuration to restrict connection to localhost

* Update send-pan test/dependencies

* Update lambdas/async operation to deploy with node 20

* Update common to export a helper to import ESM got module

* Add docstring to importGot

* Remove unneeded imports

* Minor PR review fixes

* Update CHANGELOG

* Update CHANGELOG

* Reconfigure CI to not use unsafe perms modification

* Fix broken package.json

* Fix extran. dep issue

* Make error message test less specific

* Update test to not rely on aggregate internal errors

* Update packages/ingest/test/test-HttpProviderClient.js

Co-authored-by: etcart <37375117+etcart@users.noreply.github.com>

* Move importGot -> importEsm, introduce static import definition

* Fix inadvertant test commit

* Update rules helpers to remove all null key values from rule JSON

* Update CHANGELOG

* Update helper test titles for consistency

* Minor refactor/rename

* Fix typing errors in original code, update PR accordingly

* Fix inadvertant move of rule validation

* Update packages/api/endpoints/rules.js

Co-authored-by: etcart <37375117+etcart@users.noreply.github.com>

* Remove custom null removal method in favor of omitDeepBy

* Remove duplicative call to omitDeepBy

* Update packages/api/lib/rulesHelpers.js

Co-authored-by: etcart <37375117+etcart@users.noreply.github.com>

---------

Co-authored-by: etcart <37375117+etcart@users.noreply.github.com>
Jkovarik added a commit to nasa/cumulus that referenced this issue May 7, 2024
* Update core deps to v20.12.2

* Update unit test to rmove .sort

.sort is intermittently failing to order as expected, causing this
test to fail in local test at an alarming failure rate.    Updating
test to be more explicit/remove sort, however this needs to be
investigated prior to closing thie branch/PR/ticket

* Update kinesis test to node v20

JSON.parse now throws a different error for the merged test case.

* Update knex test to handle new subdependency throwing an Aggregate
error

* Revert "Update unit test to rmove .sort"

This reverts commit adbad84.

* Update got

* Revert "Update got"

This reverts commit a19f9d1.

* Upgrade ava/nock, fix ingest provider tests

* Reapply "Update got"

This reverts commit e652516.

* Update ingest module for node 20

This update has a couple of controversial changes in it:

Updating got to v14 means we're using a pure ESM module given sindre's
stance on not supporting common exports.  That's being done due to
incompatible changes in node streams requires at least got v12

Additionally there's a probable outstanding issue in got
sindresorhus/got#2341 related to node v20/fs
readstreams/nock and/or msw incompatibility (as well as a possible
open source contrib)

* Remove httpClient test mock/fix

Updating the existing apache server to return 200 on an existing
endpoint is far better than the prior commit approach in that it's a
valid/useful unit test as a result, with the technical/less tidy
downside of requiring the unit test stack to be working.

* Update local test stack configuration to restrict connection to localhost

* Update send-pan test/dependencies

* Update lambdas/async operation to deploy with node 20

* Update common to export a helper to import ESM got module

* Add docstring to importGot

* Remove unneeded imports

* Minor PR review fixes

* Update CHANGELOG

* Update CHANGELOG

* Reconfigure CI to not use unsafe perms modification

* Fix broken package.json

* Fix extran. dep issue

* Make error message test less specific

* Update test to not rely on aggregate internal errors

* Update packages/ingest/test/test-HttpProviderClient.js



* Move importGot -> importEsm, introduce static import definition

* Fix inadvertant test commit

* Update rules helpers to remove all null key values from rule JSON

* Update CHANGELOG

* Update helper test titles for consistency

* Minor refactor/rename

* Fix typing errors in original code, update PR accordingly

* Fix inadvertant move of rule validation

* Update packages/api/endpoints/rules.js



* Remove custom null removal method in favor of omitDeepBy

* Remove duplicative call to omitDeepBy

* Update packages/api/lib/rulesHelpers.js



---------

Co-authored-by: etcart <37375117+etcart@users.noreply.github.com>
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

4 participants