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

GCS: is the correct event 'complete' or 'finish' for uploading? #849

Closed
theacodes opened this issue Sep 3, 2015 · 11 comments
Closed

GCS: is the correct event 'complete' or 'finish' for uploading? #849

theacodes opened this issue Sep 3, 2015 · 11 comments
Assignees
Labels
api: storage Issues related to the Cloud Storage API.

Comments

@theacodes
Copy link

When uploading a file, eg:

      var stream = file.createWriteStream();

      stream.on('error', function(err) {
        uploadedFile.cloudStorageError = err;
        checkNext();
      });

      stream.on('complete', function() {
        next();
      });

      stream.end(buffer);

Is the correct event complete or finish. The docs say complete, but a user reported an issue with using complete and that finish was the one that worked for him. In my testing, both work with gcloud 0.20.0.

@stephenplusplus
Copy link
Contributor

Great question and one that was recently resolved once and for all.

Answer for 0.20.0: complete
Answer for master: finish

All writable streams will emit finish when they're done, but the request module which makes all of our requests requires you to latch onto "complete" to get the full response data from the request. That's why we've had to ask users to listen to complete... we needed to be able to confirm the request was successful (confirming data integrity, mostly).

In master however, we figured out a way to not require users to listen for complete. PR: #824

Without having more details, I can't reliably guess what happened for the user who had the discrepancy. A possibility is the request finished but was later determined to have not been successful. If their script exited or carried on without waiting for complete, the "finish" event still would have fired making it look like it was successful. Feel free to send them over here though or try master directly, I'll be happy to sort it out.

@theacodes
Copy link
Author

There was an error in the branch we was working with the pinned gcloud to master instead of 0.20, which has been resolved. If someone is using master and listening for 'complete', will things still work as expected?

@theacodes
Copy link
Author

Ah, I see in the PR that it's a breaking change. We leave this open until the next release? I can update nodejs-getting-started to the new release and fix the event.

@callmehiphop
Copy link
Contributor

That sounds like a plan, I think we'll be releasing sometime early next week.

@callmehiphop callmehiphop added the api: storage Issues related to the Cloud Storage API. label Sep 3, 2015
@theacodes
Copy link
Author

Great, I'll keep an eye on this issue so please close it when you tag the release.

@callmehiphop
Copy link
Contributor

Will do!

@thalesmello
Copy link

I was the one to originally post the problem with the complete vs finish event regarding the Node tutorial.

In my tests, the complete callback wasn't getting called whereas the finish callback was. Because of the refactor storage streams pull request, I figured it was the root of my problem.

However, the version in my package.json is "gcloud": "^0.16.0". Wasn't it supposed to be working then, as it's previous to 0.20.0?

@callmehiphop
Copy link
Contributor

finish and complete signify two different things.

finish is emitted when a writeable stream has finished writing all of its data.
complete is emitted when the network request itself is completed.

As of the current stable release (and I believe all releases previous) it's possible that complete will not emit in the event that data validation failed. If this scenario were to occur, an error event would be emitted.

Could you provide a code snippet to reproduce the issue you are seeing?

Also, I know you mentioned that you have ^0.16.0 listed within your package.json, but would you mind confirming that it's the version you have installed?

@thalesmello
Copy link

@callmehiphop I've made a little bit of confusion.

At the time I got my problem, I was using a package.json that pointed to the master branch of this repository, as @jonparrott pointed out.

For some reason the package.json of my project changed to ^0.16.0, even though I don't recall manually changing that.

@jonparrott Just pinned the version in the current package version to ^0.20.0, so I believe it won't be a problem anymore.

Sorry for the confusion.

@stephenplusplus
Copy link
Contributor

@jonparrott -- 0.21.0 just released.

@theacodes
Copy link
Author

Thanks, i'll update the tutorials shortly.

sofisl pushed a commit that referenced this issue Jan 17, 2023
This PR was generated using Autosynth. 🌈

Synth log will be available here:
https://source.cloud.google.com/results/invocations/f9f34ae0-d83d-4c7a-a197-54a0a7255ebc/targets

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

Source-Link: googleapis/synthtool@15013ef
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.
Projects
None yet
Development

No branches or pull requests

4 participants