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

fix(example): drive export end before finish writing the file #1798

Merged
merged 3 commits into from Sep 12, 2019
Merged

fix(example): drive export end before finish writing the file #1798

merged 3 commits into from Sep 12, 2019

Conversation

ttv20
Copy link
Contributor

@ttv20 ttv20 commented Aug 20, 2019

Fixes #1788

  • [V] Tests and linter pass
  • [V] Code coverage does not decrease (if any source code was changed)
  • [V] Appropriate docs were updated (if necessary)

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Aug 20, 2019
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we just switch end to finish I believe? I'd love to keep the change to a minimum and not introduce the Promise.all.

Thanks for hunting this down.

@ttv20
Copy link
Contributor Author

ttv20 commented Aug 26, 2019

can't we just switch end to finish I believe? I'd love to keep the change to a minimum and not introduce the Promise.all.

Thanks for hunting this down.

the 'end' event is the end of the response (res.data) and the 'finish' event is the end of the file write stream (dest), so it not just switch.
The question is if we address potential error with the response

We can just ignore potential error with the response:

async function runSample() {
  // [START main_body]
  const fileId = '1EkgdLY3T-_9hWml0VssdDWQZLEc8qqpMB77Nvsx6khA';
  const destPath = path.join(os.tmpdir(), 'important.pdf');
  const dest = fs.createWriteStream(destPath);
  const res = await drive.files.export(
    {fileId, mimeType: 'application/pdf'},
    {responseType: 'stream'}
  );
  res.data.pipe(dest);
  await new Promise((resolve, reject) => {
    dest
    .on('finish', () => {
      console.log(`Done saving document: ${destPath}.`);
      resolve();
    })
    .on('error', err => {
      console.error('Error saving document.');
      reject(err);
    })
  })
  // [END main_body]
}

@bcoe
Copy link
Contributor

bcoe commented Aug 26, 2019

I believe finish shouldn't be called on dest until the underlying resource has been closed, and has in most cases had all data written to it. tldr; except for really weird edge-cases, I think we can get away with just switching to finish?

@bcoe
Copy link
Contributor

bcoe commented Sep 5, 2019

@ttv20 👋 still want to see this over the finish line? I think you also need to sign the CLA.

@ttv20
Copy link
Contributor Author

ttv20 commented Sep 8, 2019

I believe finish shouldn't be called on dest until the underlying resource has been closed, and has in most cases had all data written to it. tldr; except for really weird edge-cases, I think we can get away with just switching to finish?

No. 'finish' is event of stream.Writable and 'end' is event of stream.Readable so it'll not work to just change to 'finish'
I remember that I tried 'finish' and it was the same and it didn't finish to write the file, I busy this days so I don't have time to test it. (it not spouse to work according the docs, maybe finish is alias of end)

From node doc:
stream.Readable -> 'end' event:

The 'end' event is emitted when there is no more data to be consumed from the stream.

stream.Writable -> 'finish' event:

The 'finish' event is emitted after the stream.end() method has been called, and all data has been flushed to the underlying system.

So I think it necessary to wait for dest (writable stream) to finish and not miss the writing of the last chunks of data.
And I don't think it's happens only on 'really weird edge-cases', on my case I just run the example on bigger file (700KB file)

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Sep 8, 2019
@ttv20 ttv20 changed the title fix example drive export fix(example): drive export end before finish writing the file Sep 8, 2019
@bcoe
Copy link
Contributor

bcoe commented Sep 9, 2019

@ttv20 thank you for your work on this, it's much appreciated 😄 we want a sample that works!

I just chatted with my colleague @callmehiphop, his recommendation was this simplification:

await new Promise((resolve, reject) => {
  res.data
    .on('error', reject)
    .pipe(dest)
    .on('error', reject)
    .on('finish', resolve);
});

I believe this would have the desired effect, where the on after the pipe is listening on the dest not the src.

drive export end before finish writing the file
@ttv20
Copy link
Contributor Author

ttv20 commented Sep 12, 2019

Tested, Work as expected.
Create new PR?

@bcoe
Copy link
Contributor

bcoe commented Sep 12, 2019

@ttv20 this PR is great 👍 thanks for the patience.

@bcoe bcoe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2019
@codecov
Copy link

codecov bot commented Sep 12, 2019

Codecov Report

Merging #1798 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1798   +/-   ##
=======================================
  Coverage   96.51%   96.51%           
=======================================
  Files           2        2           
  Lines         431      431           
  Branches        7        7           
=======================================
  Hits          416      416           
  Misses         15       15

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc32427...961b122. Read the comment docs.

@bcoe bcoe merged commit 26ac7d8 into googleapis:master Sep 12, 2019
@bcoe
Copy link
Contributor

bcoe commented Sep 12, 2019

@ttv20 thanks a ton for the contribution 😄 and for patience in it landing.

@ttv20 ttv20 deleted the patch-3 branch September 13, 2019 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drive API: example mistake
4 participants