Emit errors from upstream on file upload #426

ryanseys · 2015-03-04T22:07:14Z

I (think I) have a fix here for #422 but writing the test for it has been troublesome for me, most likely due to my lack of understanding with streams. Hopefully @stephenplusplus can help!

The test is passing (all asserts pass and done is called) and then immediately failing due to stream.end being called on a stream that (I guess) no longer exists?

I'm getting the following error when running the test included in this PR and don't know how to fix:

> mocha test/*

  File
    createWriteStream
      ✓ should re-emit errors
      1) should re-emit errors

  1 passing (23ms)
  1 failing

  1) File createWriteStream should re-emit errors:
     Uncaught TypeError: Cannot read property 'ending' of undefined
      at Duplexify.end (/Users/ryanseys/gcloud-node/node_modules/duplexify/index.js:218:27)
      at handleError (/Users/ryanseys/gcloud-node/lib/storage/file.js:1064:12)
      at DestroyableTransform.<anonymous> (/Users/ryanseys/gcloud-node/lib/storage/file.js:992:7)
      at DestroyableTransform.emit (events.js:129:20)
      at Immediate._onImmediate (/Users/ryanseys/gcloud-node/test/storage/file.js:553:18)
      at processImmediate [as _immediateCallback] (timers.js:358:17)

test/storage/file.js

@@ -528,6 +528,44 @@ describe('File', function() {
      writable.write('data');
    });

+    it('should re-emit errors', function(done) {
+      var error = new Error('Error.');


stephenplusplus · 2015-03-05T18:14:31Z

I believe handleError doesn't need to end the stream. Errors are emitted back to the stream that was passed into startResumableUpload_, which ends the stream there.

... streams of tears.

lib/storage/file.js

@@ -1056,7 +1060,6 @@ File.prototype.startResumableUpload_ = function(stream, metadata) {
    }

    stream.emit('error', err);
-    stream.end();


ryanseys · 2015-03-05T20:14:41Z

PTAL 😄

Emit errors from upstream on file upload

stephenplusplus · 2015-03-05T20:15:36Z

👍

teddybearz · 2015-03-05T21:00:57Z

Is it possible that handleError() being called twice? i.e. one error event and one complete event with error come back to back?

ryanseys · 2015-03-05T21:26:39Z

It was that the stream was being closed too many times. I believe we have addressed this issue fully. If you want to test against master, that would be wonderful!

teddybearz · 2015-03-05T21:57:11Z

I wonder in real network scenario (not just the test mock environment), it is possible that there will be one error event and one complete event+error being emitted back to back with error code all equals to 404 or 499-600. If it happens, the handleError() will be called twice and startUpload() resumeUpload() will be called twice.

stephenplusplus · 2015-03-05T23:44:36Z

The write stream will either emit an error or complete, it can't do both.

Regardless, somewhat related, we should aim to "once-ify" our user provided callbacks.

stephenplusplus · 2015-03-05T23:47:13Z

@teddybearz I can't help but feel I'm missing your point. If I am, thanks for your patience.

teddybearz · 2015-03-06T20:04:12Z

The write stream will either emit an error or complete, it can't do both.
In theory yes, but for event driven code, it takes some efforts to enforce and maintain that. Maybe better not assume too much and prepare for the worst.

BTW, is there a mock environment I can use to test my code that uses this library? So I can simulate bad-upload/bad-download/network-connection-reset those kinds of error cases programmatically? I am tired of pulling cable now 😄

ryanseys · 2015-03-07T18:33:30Z

@teddybearz You might have some luck trying out https://github.com/tylertreat/Comcast 😄

This PR was generated using Autosynth. 🌈 Synth log will be available here: https://source.cloud.google.com/results/invocations/b742586e-df31-4aac-8092-78288e9ea8e7/targets - [ ] To automatically regenerate this PR, check this box. PiperOrigin-RevId: 325949033 Source-Link: googleapis/googleapis@94006b3

This PR was generated using Autosynth. 🌈 Synth log will be available here: https://source.cloud.google.com/results/invocations/dfbad313-7afb-4cf6-b229-0476fcc2130c/targets - [ ] To automatically regenerate this PR, check this box. Source-Link: googleapis/synthtool@57c23fa

…es (#426) PiperOrigin-RevId: 317130948 Source-Author: Google APIs <noreply@google.com> Source-Date: Thu Jun 18 10:28:49 2020 -0700 Source-Repo: googleapis/googleapis Source-Sha: eb37e688331443969eed9b969531751154a956d5 Source-Link: googleapis/googleapis@eb37e68

Source-Author: Benjamin E. Coe <bencoe@google.com> Source-Date: Wed Aug 12 12:12:29 2020 -0700 Source-Repo: googleapis/synthtool Source-Sha: 5747555f7620113d9a2078a48f4c047a99d31b3e Source-Link: googleapis/synthtool@5747555

This PR was generated using Autosynth. 🌈 Synth log will be available here: https://source.cloud.google.com/results/invocations/6097b27f-11d0-4351-8f36-1ff5504fc384/targets - [ ] To automatically regenerate this PR, check this box.

🤖 I have created a release \*beep\* \*boop\* --- ## [2.2.0](https://www.github.com/googleapis/nodejs-iot/compare/v2.1.3...v2.2.0) (2021-01-09) ### Features * adds style enumeration ([#426](https://www.github.com/googleapis/nodejs-iot/issues/426)) ([4daeff4](https://www.github.com/googleapis/nodejs-iot/commit/4daeff47d5cfc5fd826705f4da5b8dbed1784e44)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

Update npm scripts: add clean, prelint, prefix; make sure that lint and fix are set properly. Use post-process feature of synthtool.

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

ryanseys added the don't merge label Mar 4, 2015

stephenplusplus reviewed Mar 5, 2015
View reviewed changes

test/storage/file.js

@@ -528,6 +528,44 @@ describe('File', function() {

writable.write('data');

});

it('should re-emit errors', function(done) {

var error = new Error('Error.');

This comment was marked as spam.

Sign in to view

Emit errors from upstream on file upload

869a466

ryanseys reviewed Mar 5, 2015
View reviewed changes

lib/storage/file.js

@@ -1056,7 +1060,6 @@ File.prototype.startResumableUpload_ = function(stream, metadata) {

}

stream.emit('error', err);

stream.end();

This comment was marked as spam.

Sign in to view

stephenplusplus added a commit that referenced this pull request Mar 5, 2015

Merge pull request #426 from ryanseys/emit-error

751d97f

Emit errors from upstream on file upload

stephenplusplus merged commit 751d97f into googleapis:master Mar 5, 2015

ryanseys deleted the emit-error branch March 5, 2015 20:15

ryanseys removed the don't merge label Mar 5, 2015

ryanseys mentioned this pull request Mar 5, 2015

Lack of error handling while uploading a file. #422

Closed

snyk-bot mentioned this pull request Jan 29, 2022

[Snyk] Upgrade chalk from 2.4.2 to 5.0.0 RebelTat/google-cloud-node#2

Open

chingor13 pushed a commit that referenced this pull request Aug 22, 2022

test: turn on some test, turn off others (#426)

19f7b5d

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 27, 2022

chore: update lint ignore files (#426)

487069f

sofisl mentioned this pull request Sep 27, 2022

migrate code from googleapis/nodejs-video-intelligence #3374

Closed

4 tasks

sofisl mentioned this pull request Oct 12, 2022

migrate code from googleapis/nodejs-bigquery-data-transfer #3399

Merged

4 tasks

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

chore: update lint ignore files (#426)

04deb2b

This was referenced Nov 9, 2022

migrate code from googleapis/nodejs-video-intelligence #3507

Closed

migrate code from googleapis/nodejs-video-intelligence #3519

Merged

sofisl mentioned this pull request Nov 10, 2022

migrate code from googleapis/nodejs-tasks #3530

Merged

4 tasks

This was referenced Nov 11, 2022

migrate code from googleapis/nodejs-dataproc #3533

Merged

migrate code from googleapis/nodejs-iot #3544

Merged

migrate code from googleapis/nodejs-monitoring #3545

Merged

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

chore: update npm scripts and synth.py (#426)

8eeca1f

Update npm scripts: add clean, prelint, prefix; make sure that lint and fix are set properly. Use post-process feature of synthtool.

sofisl mentioned this pull request Nov 11, 2022

migrate code from googleapis/nodejs-containeranalysis #3550

Merged

4 tasks

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

test: turn on some test, turn off others (#426)

5143db7

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

chore: remove duplicate mocha config (#426)

618290d

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 pushed a commit that referenced this pull request Jan 10, 2023

build(tests): fix coveralls and enable build cop (#426)

8b28085

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit errors from upstream on file upload #426

Emit errors from upstream on file upload #426

ryanseys commented Mar 4, 2015

This comment was marked as spam.

stephenplusplus commented Mar 5, 2015

This comment was marked as spam.

ryanseys commented Mar 5, 2015

stephenplusplus commented Mar 5, 2015

teddybearz commented Mar 5, 2015

ryanseys commented Mar 5, 2015

teddybearz commented Mar 5, 2015

stephenplusplus commented Mar 5, 2015

stephenplusplus commented Mar 5, 2015

teddybearz commented Mar 6, 2015

ryanseys commented Mar 7, 2015

Emit errors from upstream on file upload #426

Emit errors from upstream on file upload #426

Conversation

ryanseys commented Mar 4, 2015

This comment was marked as spam.

stephenplusplus commented Mar 5, 2015

This comment was marked as spam.

ryanseys commented Mar 5, 2015

stephenplusplus commented Mar 5, 2015

teddybearz commented Mar 5, 2015

ryanseys commented Mar 5, 2015

teddybearz commented Mar 5, 2015

stephenplusplus commented Mar 5, 2015

stephenplusplus commented Mar 5, 2015

teddybearz commented Mar 6, 2015

ryanseys commented Mar 7, 2015