Ensure File#createReadStream sockets are properly closed on error #530

ryanseys · 2015-05-05T16:33:01Z

Trying to address #523

Don't merge this, it's not complete.

This seems to mitigate sockets staying open when a request is ended early (only scenario I tried to get the request to hang).

Question for @robertdimarco: What do you mean by:

and hang-up part-way through that file

in your comment here. I don't know how to "hang up" part way, and is it the server that is hanging up or the client? (I'm assuming client). Any other scenarios you can think of that we should test?

@stephenplusplus Seems that done at the bottom of File#createReadStream isn't called when throughStream gets an error event like you can see in this PR. I listen for this event specifically and destroy the socket related to the request at that time. This seems to destroy all the sockets laying around and fires an error events saying there is a "Download mismatch" according to verification.

My test code can be found here: https://gist.github.com/ryanseys/0e2c3244175db6525d7a

Set maxSockets: 2 when running test code because currently infinite sockets are allowed to remain open so the 4th request will succeed.

I have a feeling this is on the right track but I perhaps don't clean up everything appropriately due to the craziness that is Node.js Streams and request. In any case, it still seems to mitigate the issue a bit so far.

robertdimarco · 2015-05-05T16:53:08Z

@ryanseys I'm referring to a client disconnect before the write-stream was able to complete. We came across this issue by pointing a browser at the proxy server, let the browser connect and begin downloading, and then close the tab / refresh / etc. to cause the client to disconnect the socket. That is generally sufficient to reproduce the issue.

The handling for this I imagine is a little tricky, because the error might not be propagated from Express's write-stream to the read-stream / through-stream returned from file.createReadStream(). That part around piping likely just needs some documentation around it, as other devs will have to handle that client disconnect and propagate to this module.

ryanseys · 2015-05-05T16:58:40Z

The problem with using that scenario (i.e. closing a tab) is I have no idea what that means in regards to what happens to either the read or write stream. A code-only test to reproduce this is most ideal, especially for ensuring this issue doesn't arise again as a regression. That's what I'm trying to accomplish here with my test script. I will try your Express method and watch the events that occur on the streams to get an idea of where things can go wrong so we thoroughly patch this thing.

robertdimarco · 2015-05-05T17:15:17Z

@ryanseys Understood. Let me know if I can be of any help.

stephenplusplus · 2015-05-05T21:05:51Z

@ryanseys I have a bit of a different approach going on: stephenplusplus@7b4ab4f - can you try this out with your tests?

ryanseys · 2015-05-05T22:07:38Z

@stephenplusplus That's much cleaner and appears to work with my tests when maxSockets: Infinity (no open sockets at the end). When maxSockets: 2, weird things happen though. The third request that get's prematurely ended doesn't throw an error, or the 4th request that should succeed just never does, BUT there are no sockets left open as well. Considering we are going to have maxSockets: Infinity this shouldn't be an issue.

ryanseys · 2015-05-05T22:16:09Z

Actually that might be just because maxSockets is doing its job, in which case we just set it to Infinity and hope nobody has more than Infinity simultaneous sockets open on gcloud (but if this happens, that person should probably get a nobel prize of the node.js variety).

ryanseys · 2015-05-05T22:17:06Z

tl;dr: looks good to me. I'll cherry pick into this PR.

googlebot · 2015-05-05T22:18:29Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

ryanseys · 2015-05-05T22:21:37Z

@googlebot confirm.

ryanseys · 2015-05-05T22:23:23Z

Apparently @googlebot doesn't like you @stephenplusplus

stephenplusplus · 2015-05-05T22:47:48Z

Neither does Travis. I'm not big with robots, I guess :/

stephenplusplus · 2015-05-14T15:26:49Z

@ryanseys I suppose I should fix the tests, since I'm the one who wrecked your PR!

stephenplusplus · 2015-06-02T17:35:40Z

Carrying this on in #636.

@alexander-fenster

feat!: *Change metadata field for the AnalyzeIamPolicyLongrunning. feat: Add AnalyzeMove API. feat: Add read_mask field for SearchAllResourcesRequest feat：Add VersionedResource/AttachedResource fields for ResourceSearchResult Committer: @alexander-fenster PiperOrigin-RevId: 387841814 PiperOrigin-RevId: 387216202 PiperOrigin-RevId: 386530026

@alexander-fenster

feat!: *Change metadata field for the AnalyzeIamPolicyLongrunning. feat: Add AnalyzeMove API. feat: Add read_mask field for SearchAllResourcesRequest feat：Add VersionedResource/AttachedResource fields for ResourceSearchResult Committer: @alexander-fenster PiperOrigin-RevId: 387841814 PiperOrigin-RevId: 387216202 PiperOrigin-RevId: 386530026

@alexander-fenster

feat!: *Change metadata field for the AnalyzeIamPolicyLongrunning. feat: Add AnalyzeMove API. feat: Add read_mask field for SearchAllResourcesRequest feat：Add VersionedResource/AttachedResource fields for ResourceSearchResult Committer: @alexander-fenster PiperOrigin-RevId: 387841814 PiperOrigin-RevId: 387216202 PiperOrigin-RevId: 386530026

🤖 I have created a release \*beep\* \*boop\* --- ### [3.3.1](https://www.github.com/googleapis/nodejs-video-intelligence/compare/v3.3.0...v3.3.1) (2021-04-19) ### Bug Fixes * update region tag from v1beta1 to v1 for analyze-face-detection.js ([#530](https://www.github.com/googleapis/nodejs-video-intelligence/issues/530)) ([4d19f9d](https://www.github.com/googleapis/nodejs-video-intelligence/commit/4d19f9de9e386ee92ed18d9302cddc61c2dc9865)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [uuid](https://togithub.com/uuidjs/uuid) | devDependencies | major | [`^7.0.0` -> `^8.0.0`](https://renovatebot.com/diffs/npm/uuid/7.0.3/8.0.0) | --- ### Release Notes <details> <summary>uuidjs/uuid</summary> ### [`v8.0.0`](https://togithub.com/uuidjs/uuid/blob/master/CHANGELOG.md#800-httpsgithubcomuuidjsuuidcomparev703v800-2020-04-29) [Compare Source](https://togithub.com/uuidjs/uuid/compare/v7.0.3...v8.0.0) ##### ⚠ BREAKING CHANGES - For native ECMAScript Module (ESM) usage in Node.js only named exports are exposed, there is no more default export. ```diff -import uuid from 'uuid'; -console.log(uuid.v4()); // -> 'cd6c3b08-0adc-4f4b-a6ef-36087a1c9869' +import { v4 as uuidv4 } from 'uuid'; +uuidv4(); // ⇨ '9b1deb4d-3b7d-4bad-9bdd-2b0d7b3dcb6d' ``` - Deep requiring specific algorithms of this library like `require('uuid/v4')`, which has been deprecated in `uuid@7`, is no longer supported. Instead use the named exports that this module exports. For ECMAScript Modules (ESM): ```diff -import uuidv4 from 'uuid/v4'; +import { v4 as uuidv4 } from 'uuid'; uuidv4(); ``` For CommonJS: ```diff -const uuidv4 = require('uuid/v4'); +const { v4: uuidv4 } = require('uuid'); uuidv4(); ``` ##### Features - native Node.js ES Modules (wrapper approach) ([#423](https://togithub.com/uuidjs/uuid/issues/423)) ([2d9f590](https://togithub.com/uuidjs/uuid/commit/2d9f590ad9701d692625c07ed62f0a0f91227991)), closes [#245](https://togithub.com/uuidjs/uuid/issues/245) [#419](https://togithub.com/uuidjs/uuid/issues/419) [#342](https://togithub.com/uuidjs/uuid/issues/342) - remove deep requires ([#426](https://togithub.com/uuidjs/uuid/issues/426)) ([daf72b8](https://togithub.com/uuidjs/uuid/commit/daf72b84ceb20272a81bb5fbddb05dd95922cbba)) ##### Bug Fixes - add CommonJS syntax example to README quickstart section ([#417](https://togithub.com/uuidjs/uuid/issues/417)) ([e0ec840](https://togithub.com/uuidjs/uuid/commit/e0ec8402c7ad44b7ef0453036c612f5db513fda0)) ##### [7.0.3](https://togithub.com/uuidjs/uuid/compare/v7.0.2...v7.0.3) (2020-03-31) ##### Bug Fixes - make deep require deprecation warning work in browsers ([#409](https://togithub.com/uuidjs/uuid/issues/409)) ([4b71107](https://togithub.com/uuidjs/uuid/commit/4b71107d8c0d2ef56861ede6403fc9dc35a1e6bf)), closes [#408](https://togithub.com/uuidjs/uuid/issues/408) ##### [7.0.2](https://togithub.com/uuidjs/uuid/compare/v7.0.1...v7.0.2) (2020-03-04) ##### Bug Fixes - make access to msCrypto consistent ([#393](https://togithub.com/uuidjs/uuid/issues/393)) ([8bf2a20](https://togithub.com/uuidjs/uuid/commit/8bf2a20f3565df743da7215eebdbada9d2df118c)) - simplify link in deprecation warning ([#391](https://togithub.com/uuidjs/uuid/issues/391)) ([bb2c8e4](https://togithub.com/uuidjs/uuid/commit/bb2c8e4e9f4c5f9c1eaaf3ea59710c633cd90cb7)) - update links to match content in readme ([#386](https://togithub.com/uuidjs/uuid/issues/386)) ([44f2f86](https://togithub.com/uuidjs/uuid/commit/44f2f86e9d2bbf14ee5f0f00f72a3db1292666d4)) ##### [7.0.1](https://togithub.com/uuidjs/uuid/compare/v7.0.0...v7.0.1) (2020-02-25) ##### Bug Fixes - clean up esm builds for node and browser ([#383](https://togithub.com/uuidjs/uuid/issues/383)) ([59e6a49](https://togithub.com/uuidjs/uuid/commit/59e6a49e7ce7b3e8fb0f3ee52b9daae72af467dc)) - provide browser versions independent from module system ([#380](https://togithub.com/uuidjs/uuid/issues/380)) ([4344a22](https://togithub.com/uuidjs/uuid/commit/4344a22e7aed33be8627eeaaf05360f256a21753)), closes [#378](https://togithub.com/uuidjs/uuid/issues/378) </details> --- ### Renovate configuration :date: **Schedule**: "after 9am and before 3pm" (UTC). :vertical_traffic_light: **Automerge**: Disabled by config. Please merge this manually once you are satisfied. :recycle: **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. :no_bell: **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#googleapis/nodejs-translate).

🤖 I have created a release \*beep\* \*boop\* --- ### [2.3.2](https://www.github.com/googleapis/nodejs-tasks/compare/v2.3.1...v2.3.2) (2021-05-25) ### Bug Fixes * GoogleAdsError missing using generator version after 1.3.0 ([#529](https://www.github.com/googleapis/nodejs-tasks/issues/529)) ([760c204](https://www.github.com/googleapis/nodejs-tasks/commit/760c2043f9bccb0d2787e83dd08ace942e6b10fd)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

…530) This reverts commit e1557e468fd986c952ba718d9ff90e1d87390209. Source-Link: googleapis/synthtool@8a475dc Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:ba0957cb15a1b8ca7ec2795c7783cd09cb68be2de9f4a7c69aa15b759c622735 Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

[![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [ts-loader](https://togithub.com/TypeStrong/ts-loader) | [`^8.0.0` -> `^9.0.0`](https://renovatebot.com/diffs/npm/ts-loader/8.1.0/9.0.0) | [![age](https://badges.renovateapi.com/packages/npm/ts-loader/9.0.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/npm/ts-loader/9.0.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/npm/ts-loader/9.0.0/compatibility-slim/8.1.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/npm/ts-loader/9.0.0/confidence-slim/8.1.0)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>TypeStrong/ts-loader</summary> ### [`v9.0.0`](https://togithub.com/TypeStrong/ts-loader/blob/master/CHANGELOG.md#v900) [Compare Source](https://togithub.com/TypeStrong/ts-loader/compare/v8.1.0...v9.0.0) Breaking changes: - minimum webpack version: 5 - minimum node version: 12 Changes: - [webpack 5 migration](https://togithub.com/TypeStrong/ts-loader/pull/1251) - thanks [@johnnyreilly](https://togithub.com/johnnyreilly), [@jonwallsten](https://togithub.com/jonwallsten), [@sokra](https://togithub.com/sokra), [@appzuka](https://togithub.com/appzuka), [@alexander-akait](https://togithub.com/alexander-akait) </details> --- ### Configuration :date: **Schedule**: "after 9am and before 3pm" (UTC). :vertical_traffic_light: **Automerge**: Disabled by config. Please merge this manually once you are satisfied. :recycle: **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. :no_bell: **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box. --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/nodejs-monitoring).

@alexander-fenster

feat!: *Change metadata field for the AnalyzeIamPolicyLongrunning. feat: Add AnalyzeMove API. feat: Add read_mask field for SearchAllResourcesRequest feat：Add VersionedResource/AttachedResource fields for ResourceSearchResult Committer: @alexander-fenster PiperOrigin-RevId: 387841814 PiperOrigin-RevId: 387216202 PiperOrigin-RevId: 386530026

…mplate provided (#530) Fixes #529

…530) This PR was generated using Autosynth. 🌈 Synth log will be available here: https://source.cloud.google.com/results/invocations/157de3e1-dc75-4d92-b3d9-35d704dfd201/targets - [ ] To automatically regenerate this PR, check this box. Source-Link: googleapis/synthtool@8cf6d28

🤖 I have created a release *beep* *boop* --- ## [3.0.1](googleapis/nodejs-dns@v3.0.0...v3.0.1) (2022-06-09) ### Bug Fixes * **deps:** update dependency @google-cloud/common to v4 ([#529](googleapis/nodejs-dns#529)) ([4e8b02f](googleapis/nodejs-dns@4e8b02f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

Destroy socket on throughStream error

a812561

ryanseys added the don't merge label May 5, 2015

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 5, 2015

die this way die?

f7ef2c1

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels May 5, 2015

ryanseys changed the title ~~Destroy socket on throughStream error~~ Ensure File#createReadStream sockets are properly closed on error May 5, 2015

stephenplusplus mentioned this pull request Jun 2, 2015

storage: close sockets on error #636

Merged

1 task

stephenplusplus closed this Jun 2, 2015

ryanseys deleted the die-socket-die branch June 2, 2015 20:31

sofisl mentioned this pull request Sep 15, 2022

migrate code from googleapis/nodejs-redis #3360

Merged

4 tasks

sofisl pushed a commit that referenced this pull request Sep 15, 2022

build: add generated samples to .eslintignore (#530)

14cf628

sofisl mentioned this pull request Nov 10, 2022

migrate code from googleapis/nodejs-translate #3520

Merged

4 tasks

sofisl mentioned this pull request Nov 10, 2022

migrate code from googleapis/nodejs-tasks #3530

Merged

4 tasks

sofisl mentioned this pull request Nov 11, 2022

migrate code from googleapis/nodejs-cloud-container #3543

Merged

4 tasks

sofisl mentioned this pull request Nov 11, 2022

migrate code from googleapis/nodejs-monitoring #3545

Merged

4 tasks

sofisl mentioned this pull request Nov 11, 2022

migrate code from googleapis/nodejs-asset #3567

Merged

4 tasks

sofisl pushed a commit that referenced this pull request Nov 17, 2022

fix(createVM): do not define machineType or networkInterfaces when te…

ae93a1e

…mplate provided (#530) Fixes #529

sofisl mentioned this pull request Nov 17, 2022

migrate code from googleapis/nodejs-compute #3666

Merged

4 tasks

sofisl mentioned this pull request Jan 10, 2023

migrate code from googleapis/nodejs-dlp #3837

Merged

4 tasks

sofisl mentioned this pull request Jan 17, 2023

migrate code from googleapis/nodejs-vision #3860

Merged

4 tasks

sofisl pushed a commit that referenced this pull request Jan 17, 2023

feat: bump release level to GA (#530)

abe82d0

sofisl mentioned this pull request Sep 14, 2023

migrate code from googleapis/nodejs-dns #4657

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure File#createReadStream sockets are properly closed on error #530

Ensure File#createReadStream sockets are properly closed on error #530

ryanseys commented May 5, 2015

robertdimarco commented May 5, 2015

ryanseys commented May 5, 2015

robertdimarco commented May 5, 2015

stephenplusplus commented May 5, 2015

ryanseys commented May 5, 2015

ryanseys commented May 5, 2015

ryanseys commented May 5, 2015

googlebot commented May 5, 2015

ryanseys commented May 5, 2015

ryanseys commented May 5, 2015

stephenplusplus commented May 5, 2015

stephenplusplus commented May 14, 2015

stephenplusplus commented Jun 2, 2015

Ensure File#createReadStream sockets are properly closed on error #530

Ensure File#createReadStream sockets are properly closed on error #530

Conversation

ryanseys commented May 5, 2015

robertdimarco commented May 5, 2015

ryanseys commented May 5, 2015

robertdimarco commented May 5, 2015

stephenplusplus commented May 5, 2015

ryanseys commented May 5, 2015

ryanseys commented May 5, 2015

ryanseys commented May 5, 2015

googlebot commented May 5, 2015

ryanseys commented May 5, 2015

ryanseys commented May 5, 2015

stephenplusplus commented May 5, 2015

stephenplusplus commented May 14, 2015

stephenplusplus commented Jun 2, 2015