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

http response from google may not always has 'x-goog-hash' header #423

Closed
teddybearz opened this issue Mar 3, 2015 · 9 comments · Fixed by #443
Closed

http response from google may not always has 'x-goog-hash' header #423

teddybearz opened this issue Mar 3, 2015 · 9 comments · Fixed by #443
Assignees
Labels
api: storage Issues related to the Cloud Storage API. 🚨 This issue needs some love. triage me I really want to be triaged.

Comments

@teddybearz
Copy link

The http response from google may not always has 'x-goog-hash' header. When it happens, the node process will exit.

Need fix like:

diff --git a/node_modules/gcloud/lib/storage/file.js b/node_modules/gcloud/lib/storage/file.js
index 8e1a26e..4aaec72 100644
--- a/node_modules/gcloud/lib/storage/file.js
+++ b/node_modules/gcloud/lib/storage/file.js
@@ -361,10 +361,12 @@
             var md5Fail = true;

             var hashes = {};
-            res.headers['x-goog-hash'].split(',').forEach(function(hash) {
-              var hashType = hash.split('=')[0];
-              hashes[hashType] = hash.substr(hash.indexOf('=') + 1);
-            });
+            if (res.headers && res.headers['x-goog-hash']) {
+              res.headers['x-goog-hash'].split(',').forEach(function(hash) {
+                var hashType = hash.split('=')[0];
+                hashes[hashType] = hash.substr(hash.indexOf('=') + 1);
+              });
+            }

             var remoteMd5 = hashes.md5;
             var remoteCrc = hashes.crc32c && hashes.crc32c.substr(4);
@stephenplusplus
Copy link
Contributor

Did you run into a situation where it wasn't returned?

Google Cloud Storage stores MD5 hashes for all non-composite objects. CRC32Cs are available for all objects.

@ryanseys
Copy link
Contributor

ryanseys commented Mar 5, 2015

Seems like a reasonable request for a sanity check.

@stephenplusplus
Copy link
Contributor

Still would like to know if this can come up. Would affect more of our code, if we can't always trust that a CRC32C hash is available.

@teddybearz
Copy link
Author

It happens reliably if you try to fetch an object right before the upload of the object is done.

@ryanseys
Copy link
Contributor

Why would you try to fetch an object before it was uploaded? Can you provide a snippet of code that causes this issue?

@stephenplusplus stephenplusplus added the api: storage Issues related to the Cloud Storage API. label Mar 10, 2015
@teddybearz
Copy link
Author

It was not intentionally. The bug was triggered because of in my old code, I registered on the "finish" event instead of "complete" event. A normal writeablestream will emit 'finish' when the data has been flushed to the underlying system, apparently 'finish' and 'complete' have different semantic in your lib. 'finish' event doesn't mean the object is committed. There is no document about this 'complete" event and I have to dig into the lib code to find out that.

Nevertheless, the lib's robustness should not entirely depend on the well-behave of the backend service. A not-well-behaved service should not cause the lib user's process exits.

@stephenplusplus
Copy link
Contributor

Regarding finish vs complete, it's just kind of hard to work with Node streams. You'll see various libraries using different terms for different reasons. We use complete to be consistent with the request library, which is a familiar term for many developers. That at least makes the transition a little less painful for most.

Regarding the header, it should still always be there. I don't think we need to defensively program against that, since that the code pasted in your initial post executes after we've received a response from an upload. I'm still uncertain how our library executed that code before the response came back. Code that you used showing that happening may help me understand better.

@teddybearz
Copy link
Author

To reproduce that you just need to fetch the object right after get the 'finish' event during a stream upload.

fsStream.pipe(bucket.file(blobname).createWriteStream({ metadata: metadata} )).
.on('error', function(err) {
handleError(err, 'write');
})
.on('finish', function(ex) {
if (!fdCbCalled) {
fdCbCalled = true;
future.return();
}
});
future.wait();
fetchBlob(blobName);

50% of the time you will get a response without the 'x-goog-hash' header the lib code expects.

@stephenplusplus
Copy link
Contributor

We merged a PR that will hopefully help with explaining we use complete and not finish. I believe that was one part of this issue, and the other was: in parallel, upload a file and read from it. @ryanseys did some testing and observed the API responds with a 404 error, however, our code doesn't account for this and continues on as if it's a successful response.

Specifically, we need to implement util.handleResp here to help determine the quality of the API response.

Thanks for catching this @teddybearz and for tracking down the problem @ryanseys! PR incoming :)

@yoshi-automation yoshi-automation added 🚨 This issue needs some love. triage me I really want to be triaged. labels Apr 7, 2020
sofisl pushed a commit that referenced this issue Sep 15, 2022
sofisl pushed a commit that referenced this issue Sep 27, 2022
sofisl pushed a commit that referenced this issue Oct 12, 2022
- [ ] Regenerate this pull request now.

PiperOrigin-RevId: 422607515

Source-Link: googleapis/googleapis@ba2ffd6

Source-Link: googleapis/googleapis-gen@73ba4ad
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiNzNiYTRhZGQyMzlhNjE5ZGE1NjdmZmJkNGU1NzMwZmRkNmRlMDRkMyJ9
sofisl pushed a commit that referenced this issue Nov 9, 2022
sofisl pushed a commit that referenced this issue Nov 10, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [mocha](https://mochajs.org/) ([source](https://togithub.com/mochajs/mocha)) | devDependencies | major | [`^7.0.0` -> `^8.0.0`](https://renovatebot.com/diffs/npm/mocha/7.2.0/8.0.1) |

---

### Release Notes

<details>
<summary>mochajs/mocha</summary>

### [`v8.0.1`](https://togithub.com/mochajs/mocha/blob/master/CHANGELOG.md#&#8203;801--2020-06-10)

[Compare Source](https://togithub.com/mochajs/mocha/compare/v8.0.0...v8.0.1)

The obligatory patch after a major.

#### 🐛 Fixes

-   [#&#8203;4328](https://togithub.com/mochajs/mocha/issues/4328): Fix `--parallel` when combined with `--watch` ([**@&#8203;boneskull**](https://togithub.com/boneskull))

### [`v8.0.0`](https://togithub.com/mochajs/mocha/blob/master/CHANGELOG.md#&#8203;800--2020-06-10)

[Compare Source](https://togithub.com/mochajs/mocha/compare/v7.2.0...v8.0.0)

In this major release, Mocha adds the ability to _run tests in parallel_. Better late than never! Please note the **breaking changes** detailed below.

Let's welcome [**@&#8203;giltayar**](https://togithub.com/giltayar) and [**@&#8203;nicojs**](https://togithub.com/nicojs) to the maintenance team!

#### 💥 Breaking Changes

-   [#&#8203;4164](https://togithub.com/mochajs/mocha/issues/4164): **Mocha v8.0.0 now requires Node.js v10.0.0 or newer.** Mocha no longer supports the Node.js v8.x line ("Carbon"), which entered End-of-Life at the end of 2019 ([**@&#8203;UlisesGascon**](https://togithub.com/UlisesGascon))

-   [#&#8203;4175](https://togithub.com/mochajs/mocha/issues/4175): Having been deprecated with a warning since v7.0.0, **`mocha.opts` is no longer supported** ([**@&#8203;juergba**](https://togithub.com/juergba))

    ✨ **WORKAROUND:** Replace `mocha.opts` with a [configuration file](https://mochajs.org/#configuring-mocha-nodejs).

-   [#&#8203;4260](https://togithub.com/mochajs/mocha/issues/4260): Remove `enableTimeout()` (`this.enableTimeout()`) from the context object ([**@&#8203;craigtaub**](https://togithub.com/craigtaub))

    ✨ **WORKAROUND:** Replace usage of `this.enableTimeout(false)` in your tests with `this.timeout(0)`.

-   [#&#8203;4315](https://togithub.com/mochajs/mocha/issues/4315): The `spec` option no longer supports a comma-delimited list of files ([**@&#8203;juergba**](https://togithub.com/juergba))

    ✨ **WORKAROUND**: Use an array instead (e.g., `"spec": "foo.js,bar.js"` becomes `"spec": ["foo.js", "bar.js"]`).

-   [#&#8203;4309](https://togithub.com/mochajs/mocha/issues/4309): Drop support for Node.js v13.x line, which is now End-of-Life ([**@&#8203;juergba**](https://togithub.com/juergba))

-   [#&#8203;4282](https://togithub.com/mochajs/mocha/issues/4282): `--forbid-only` will throw an error even if exclusive tests are avoided via `--grep` or other means ([**@&#8203;arvidOtt**](https://togithub.com/arvidOtt))

-   [#&#8203;4223](https://togithub.com/mochajs/mocha/issues/4223): The context object's `skip()` (`this.skip()`) in a "before all" (`before()`) hook will no longer execute subsequent sibling hooks, in addition to hooks in child suites ([**@&#8203;juergba**](https://togithub.com/juergba))

-   [#&#8203;4178](https://togithub.com/mochajs/mocha/issues/4178): Remove previously soft-deprecated APIs ([**@&#8203;wnghdcjfe**](https://togithub.com/wnghdcjfe)):
    -   `Mocha.prototype.ignoreLeaks()`
    -   `Mocha.prototype.useColors()`
    -   `Mocha.prototype.useInlineDiffs()`
    -   `Mocha.prototype.hideDiff()`

#### 🎉 Enhancements

-   [#&#8203;4245](https://togithub.com/mochajs/mocha/issues/4245): Add ability to run tests in parallel for Node.js (see [docs](https://mochajs.org/#parallel-tests)) ([**@&#8203;boneskull**](https://togithub.com/boneskull))

    ❗ See also [#&#8203;4244](https://togithub.com/mochajs/mocha/issues/4244); [Root Hook Plugins (docs)](https://mochajs.org/#root-hook-plugins) -- _root hooks must be defined via Root Hook Plugins to work in parallel mode_

-   [#&#8203;4304](https://togithub.com/mochajs/mocha/issues/4304): `--require` now works with ES modules ([**@&#8203;JacobLey**](https://togithub.com/JacobLey))

-   [#&#8203;4299](https://togithub.com/mochajs/mocha/issues/4299): In some circumstances, Mocha can run ES modules under Node.js v10 -- _use at your own risk!_ ([**@&#8203;giltayar**](https://togithub.com/giltayar))

#### 📖 Documentation

-   [#&#8203;4246](https://togithub.com/mochajs/mocha/issues/4246): Add documentation for parallel mode and Root Hook plugins ([**@&#8203;boneskull**](https://togithub.com/boneskull))

#### 🐛 Fixes

(All bug fixes in Mocha v8.0.0 are also breaking changes, and are listed above)

</details>

---

### Renovate configuration

📅 **Schedule**: "after 9am and before 3pm" (UTC).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻️ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **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#googleapis/nodejs-tasks).
sofisl pushed a commit that referenced this issue Nov 11, 2022
This PR was generated using Autosynth. 🌈

Synth log will be available here:
https://source.cloud.google.com/results/invocations/4de22315-84b1-493d-8da2-dfa7688128f5/targets

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

Source-Link: googleapis/synthtool@94421c4
sofisl pushed a commit that referenced this issue Nov 11, 2022
[![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [@types/sinon](https://togithub.com/DefinitelyTyped/DefinitelyTyped) | [`^9.0.0` -> `^10.0.0`](https://renovatebot.com/diffs/npm/@types%2fsinon/9.0.11/10.0.0) | [![age](https://badges.renovateapi.com/packages/npm/@types%2fsinon/10.0.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/npm/@types%2fsinon/10.0.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/npm/@types%2fsinon/10.0.0/compatibility-slim/9.0.11)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/npm/@types%2fsinon/10.0.0/confidence-slim/9.0.11)](https://docs.renovatebot.com/merge-confidence/) |

---

### 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-cloud-container).
sofisl pushed a commit that referenced this issue Nov 11, 2022
sofisl pushed a commit that referenced this issue Nov 11, 2022
See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
sofisl pushed a commit that referenced this issue Nov 17, 2022
* Change triggered by none of the following:
This git repo (https://github.com/googleapis/nodejs-compute.git)
Git repo https://github.com/googleapis/synthtool.git

* build: set AUTOSYNTH_MULTIPLE_COMMITS=true for context aware commits (#422)

googleapis/nodejs-compute@2484e4b
commit 2484e4b6521fdc9dce0c80b6e8bb4f3da7baf7df
Author: Benjamin E. Coe <bencoe@google.com>
Date:   Tue Mar 31 18:42:26 2020 -0700

    build: set AUTOSYNTH_MULTIPLE_COMMITS=true for context aware commits (#422)

* docs: document the release schedule we follow (#454)

googleapis/synthtool@6a17abc
commit 6a17abc7652e2fe563e1288c6e8c23fc260dda97
Author: Benjamin E. Coe <bencoe@google.com>
Date:   Mon Mar 23 19:22:34 2020 -0700

    docs: document the release schedule we follow (#454)

* fix: do not run node 8 CI (#456)

googleapis/synthtool@1b4cc80
commit 1b4cc80a7aaf164f6241937dd87f3bd1f4149e0c
Author: Alexander Fenster <fenster@google.com>
Date:   Wed Mar 25 08:01:31 2020 -0700

    fix: do not run node 8 CI (#456)

* fix: update template files for Node.js libraries (#463)

googleapis/synthtool@9982024
commit 99820243d348191bc9c634f2b48ddf65096285ed
Author: Alexander Fenster <fenster@google.com>
Date:   Tue Mar 31 11:56:27 2020 -0700

    fix: update template files for Node.js libraries (#463)
sofisl pushed a commit that referenced this issue Jan 24, 2023
sofisl pushed a commit that referenced this issue Jan 25, 2023
sofisl pushed a commit that referenced this issue Sep 14, 2023
…423)

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
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants