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

configStore.get(that.name) returns undefined causes uncaught exception #437

Closed
teddybearz opened this issue Mar 10, 2015 · 4 comments · Fixed by #447
Closed

configStore.get(that.name) returns undefined causes uncaught exception #437

teddybearz opened this issue Mar 10, 2015 · 4 comments · Fixed by #447
Assignees
Labels
api: storage Issues related to the Cloud Storage API. 🚨 This issue needs some love. triage me I really want to be triaged. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@teddybearz
Copy link

[31merror 2015-03-10T18:05:24.806Z: 5c00c7dd3ce2 pid:560 uncaughtExceptionHandler (requirements/uncaughtExceptionHandler.js:15) Aborting on uncaught top-level exception:  TypeError: Cannot read property 'firstChunk' of undefined
    at DestroyableTransform._transform (/opt/bb/experiments/build/node_modules/gcloud/lib/storage/file.js:866:58)
    at DestroyableTransform.Transform._read (/opt/bb/experiments/build/node_modules/gcloud/node_modules/through2/node_modules/readable-stream/lib/_stream_transform.js:184:10)
    at DestroyableTransform.Transform._write (/opt/bb/experiments/build/node_modules/gcloud/node_modules/through2/node_modules/readable-stream/lib/_stream_transform.js:172:12)
    at doWrite (/opt/bb/experiments/build/node_modules/gcloud/node_modules/through2/node_modules/readable-stream/lib/_stream_writable.js:237:10)
    at writeOrBuffer (/opt/bb/experiments/build/node_modules/gcloud/node_modules/through2/node_modules/readable-stream/lib/_stream_writable.js:227:5)
    at DestroyableTransform.Writable.write (/opt/bb/experiments/build/node_modules/gcloud/node_modules/through2/node_modules/readable-stream/lib/_stream_writable.js:194:11)
    at write (/opt/bb/experiments/build/node_modules/gcloud/node_modules/through2/node_modules/readable-stream/lib/_stream_readable.js:623:24)
    at flow (/opt/bb/experiments/build/node_modules/gcloud/node_modules/through2/node_modules/readable-stream/lib/_stream_readable.js:632:7)
    at /opt/bb/experiments/build/node_modules/gcloud/node_modules/through2/node_modules/readable-stream/lib/_stream_readable.js:600:7
    at process._tickCallback (node.js:419:13)[39m
```
@teddybearz
Copy link
Author

One possible cause is that the startupUpload()->makeAuthorizedRequest()'s error handler is handleError(), which can call setTimeout(resumeUpload) if the err.code is between > 499 and < 600. And skip the configStore.set(that.name).

One fix is: if handleError() is triggered by the startupUpload(), it should never trigger resumeUpload().

@teddybearz
Copy link
Author

I think I know the cause. The file.js use configure store ('gcloud-node') to keep track of the object upload process. If there are multiple node processes running at the same time, they may overwrite others' config file and cause problems. This effectively prohibit multiple node process running (under one user) to access the GCS at the same time. This is a serious limitation and is not documented anywhere.

I don't understand this design. Why do you need to use persist configure store to keep track every object's upload process? Why cannot you use a simple in-memory-only object to do that? Resumable upload is nice to have, but do you really need to think resumable-upload cross node process restart (at a such high expense)?

@teddybearz
Copy link
Author

I make some changes like below and my stress test that uses multiple node processes is now happy.

diff --git a/node_modules/gcloud/lib/storage/file.js b/node_modules/gcloud/lib/storage/file.js
index 7c04437..adfdf35 100644
--- a/node_modules/gcloud/lib/storage/file.js
+++ b/node_modules/gcloud/lib/storage/file.js
@@ -21,7 +21,14 @@
 'use strict';

 var bufferEqual = require('buffer-equal');
-var ConfigStore = require('configstore');
+//var ConfigStore = require('configstore');
+var all = {};
+var configStore = {
+   all: {},
+   get: function(key) { return all[key]; },
+   del: function(key) { delete all[key]; },
+   set: function(key, value) { all[key] = value; },
+};
 var crc = require('fast-crc32c');
 var crypto = require('crypto');
 var duplexify = require('duplexify');
@@ -766,7 +773,7 @@ File.prototype.startResumableUpload_ = function(stream, metadata) {
   metadata = metadata || {};

   var that = this;
-  var configStore = new ConfigStore('gcloud-node');
+  //var configStore = new ConfigStore('gcloud-node');
   var config = configStore.get(that.name);
   var makeAuthorizedRequest = that.bucket.storage.makeAuthorizedRequest_;

@ryanseys
Copy link
Contributor

I suppose the real important question here is: Do we really need to support resumable across node process restarts? I'm going to assume yes, so we just likely need better error checking to ensure that the value exists at the time we need to use it, otherwise resumable will not be used.

@jgeewax jgeewax added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. api: storage Issues related to the Cloud Storage API. labels May 14, 2015
@jgeewax jgeewax added this to the Storage Stable milestone May 14, 2015
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. triage me I really want to be triaged. labels Apr 6, 2020
chingor13 pushed a commit that referenced this issue Aug 22, 2022
🤖 I have created a release \*beep\* \*boop\* 
---
## [3.10.0](https://www.github.com/googleapis/nodejs-asset/compare/v3.9.1...v3.10.0) (2020-12-02)


### Features

* added support OSInventory in Assets ([#436](https://www.github.com/googleapis/nodejs-asset/issues/436)) ([68298fd](https://www.github.com/googleapis/nodejs-asset/commit/68298fd9ebee3ae119bdb7dc6b848276804e2cda))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
sofisl pushed a commit that referenced this issue Sep 15, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [webpack](https://togithub.com/webpack/webpack) | devDependencies | major | [`^4.41.2` -> `^5.0.0`](https://renovatebot.com/diffs/npm/webpack/4.44.2/5.1.0) |

---

### Release Notes

<details>
<summary>webpack/webpack</summary>

### [`v5.1.0`](https://togithub.com/webpack/webpack/releases/v5.1.0)

[Compare Source](https://togithub.com/webpack/webpack/compare/v5.0.0...v5.1.0)

### Features

-   expose `webpack` property from `Compiler`
-   expose `cleverMerge`, `EntryOptionPlugin`, `DynamicEntryPlugin`

### Bugfixes

-   missing `require("..").xxx` in try-catch produces a warning instead of an error now
-   handle reexports in concatenated modules correctly when they are side-effect-free
-   fix incorrect deprecation message for ModuleTemplate.hooks.hash

### [`v5.0.0`](https://togithub.com/webpack/webpack/releases/v5.0.0)

[Compare Source](https://togithub.com/webpack/webpack/compare/v4.44.2...v5.0.0)

[Announcement and changelog](https://webpack.js.org/blog/2020-10-10-webpack-5-release/)

</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.

---

 - [ ] <!-- rebase-check -->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-redis).
sofisl pushed a commit that referenced this issue Oct 12, 2022
🤖 I have created a release *beep* *boop*
---


## [3.1.0](googleapis/nodejs-resource-manager@v3.0.0...v3.1.0) (2022-03-23)


### Features

* promote to stable ([#436](googleapis/nodejs-resource-manager#436)) ([7aa1961](googleapis/nodejs-resource-manager@7aa1961)), closes [#421](googleapis/nodejs-resource-manager#421)

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
sofisl pushed a commit that referenced this issue Nov 10, 2022
This PR was generated using Autosynth. 🌈

Synth log will be available here:
https://source.cloud.google.com/results/invocations/96fb0e9d-e02a-4451-878f-e646368369cc/targets

- [ ] To automatically regenerate this PR, check this box.

Source-Link: googleapis/synthtool@94421c4
sofisl pushed a commit that referenced this issue Nov 10, 2022
This PR was generated using Autosynth. 🌈

Synth log will be available here:
https://source.cloud.google.com/results/invocations/cc99acfa-05b8-434b-9500-2f6faf2eaa02/targets

- [ ] To automatically regenerate this PR, check this box.

Source-Link: googleapis/synthtool@799d8e6
sofisl pushed a commit that referenced this issue Nov 11, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [webpack-cli](https://togithub.com/webpack/webpack-cli) | devDependencies | major | [`^3.3.10` -> `^4.0.0`](https://renovatebot.com/diffs/npm/webpack-cli/3.3.12/4.0.0) |

---

### Release Notes

<details>
<summary>webpack/webpack-cli</summary>

### [`v4.0.0`](https://togithub.com/webpack/webpack-cli/blob/master/CHANGELOG.md#&#8203;400-httpsgithubcomwebpackwebpack-clicomparewebpack-cli400-rc1webpack-cli400-2020-10-10)

[Compare Source](https://togithub.com/webpack/webpack-cli/compare/v3.3.12...webpack-cli@4.0.0)

##### Bug Fixes

-   add compilation lifecycle in watch instance ([#&#8203;1903](https://togithub.com/webpack/webpack-cli/issues/1903)) ([02b6d21](https://togithub.com/webpack/webpack-cli/commit/02b6d21eaa20166a7ed37816de716b8fc22b756a))
-   cleanup `package-utils` package ([#&#8203;1822](https://togithub.com/webpack/webpack-cli/issues/1822)) ([fd5b92b](https://togithub.com/webpack/webpack-cli/commit/fd5b92b3cd40361daec5bf4486e455a41f4c9738))
-   cli-executer supplies args further up ([#&#8203;1904](https://togithub.com/webpack/webpack-cli/issues/1904)) ([097564a](https://togithub.com/webpack/webpack-cli/commit/097564a851b36b63e0a6bf88144997ef65aa057a))
-   exit code for validation errors ([59f6303](https://togithub.com/webpack/webpack-cli/commit/59f63037fcbdbb8934b578b9adf5725bc4ae1235))
-   exit process in case of schema errors ([71e89b4](https://togithub.com/webpack/webpack-cli/commit/71e89b4092d953ea587cc4f606451ab78cbcdb93))

##### Features

-   assign config paths in build dependencies in cache config ([#&#8203;1900](https://togithub.com/webpack/webpack-cli/issues/1900)) ([7e90f11](https://togithub.com/webpack/webpack-cli/commit/7e90f110b119f36ef9def4f66cf4e17ccf1438cd))

</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.

---

 - [ ] <!-- rebase-check -->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-dataproc).
sofisl pushed a commit that referenced this issue Nov 11, 2022
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
sofisl pushed a commit that referenced this issue Nov 11, 2022
- [ ] Regenerate this pull request now.

Committer: @summer-ji-eng
PiperOrigin-RevId: 434859890

Source-Link: googleapis/googleapis@bc2432d

Source-Link: googleapis/googleapis-gen@930b673
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiOTMwYjY3MzEwM2U5MjUyM2Y4Y2ZlZDM4ZGVjZDdkM2FmYWU4ZWJlNyJ9
sofisl pushed a commit that referenced this issue Nov 11, 2022
🤖 I have created a release \*beep\* \*boop\* 
---
## [3.10.0](https://www.github.com/googleapis/nodejs-asset/compare/v3.9.1...v3.10.0) (2020-12-02)


### Features

* added support OSInventory in Assets ([#436](https://www.github.com/googleapis/nodejs-asset/issues/436)) ([68298fd](https://www.github.com/googleapis/nodejs-asset/commit/68298fd9ebee3ae119bdb7dc6b848276804e2cda))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
sofisl pushed a commit that referenced this issue Nov 18, 2022
* docs: fix docstring formatting

Committer: @parthea
PiperOrigin-RevId: 408564402

Source-Link: googleapis/googleapis@3020af5

Source-Link: googleapis/googleapis-gen@799c505
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiNzk5YzUwNTAxNGNiYWQ5OTllMmJkM2RjYTRmNWUxZjY4YmRlNWFhMiJ9

* 🦉 Updates from OwlBot

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
sofisl pushed a commit that referenced this issue Jan 17, 2023
sofisl pushed a commit that referenced this issue Sep 14, 2023
This PR was generated using Autosynth. 🌈

Synth log will be available here:
https://source.cloud.google.com/results/invocations/6fe1a2b3-59cc-42e8-a9d9-5ebb537fe61c/targets

- [ ] To automatically regenerate this PR, check this box.

Source-Link: googleapis/synthtool@ba9918c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. 🚨 This issue needs some love. triage me I really want to be triaged. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants