Skip to content
This repository was archived by the owner on Dec 16, 2025. It is now read-only.

fix(streams): reroute boundary insertion through transform stream#67

Merged
JustinBeckwith merged 7 commits intogoogleapis:masterfrom
AVaksman:partial_upload_fix
Jan 22, 2019
Merged

fix(streams): reroute boundary insertion through transform stream#67
JustinBeckwith merged 7 commits intogoogleapis:masterfrom
AVaksman:partial_upload_fix

Conversation

@AVaksman
Copy link
Copy Markdown
Contributor

Fixes googleapis/google-api-nodejs-client/issues/1354

  • Tests and linter pass

Reroute boundary insertion to the the end of the stream via transform stream.

  • To keep functionality separate, introduce a BoundaryStream class as extension of transform stream.
  • Pass boundary in the _flush method of the BoundaryStream.
  • Eliminate the need to manually close rStream.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 28, 2018
@AVaksman
Copy link
Copy Markdown
Contributor Author

If it is OK to combine the functionality, then can add _flush to the ProgressStream class.

@AVaksman
Copy link
Copy Markdown
Contributor Author

Will follow with Unit Test.

@ajaaym ajaaym added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 28, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 28, 2018
Copy link
Copy Markdown
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

I'm cool with this, but this is a very sensitive piece of code :) Please make sure before this lands that you:

  • Add a unit test to ensure the desired behavior
  • npm link this change into google-api-nodejs-client, and run through the file multipart upload samples to make sure everything still works. Once that's done - happy to approve!

Comment thread src/apirequest.ts Outdated
@JustinBeckwith
Copy link
Copy Markdown
Contributor

👋 any story on tests here? Would be awesome to see :)

@JustinBeckwith JustinBeckwith changed the title Reroute boundary insertion through transform stream fix(streams): reroute boundary insertion through transform stream Jan 15, 2019
Comment thread src/apirequest.ts Outdated
const finale = `--${boundary}--`;
const rStream = new stream.PassThrough();
const pStream = new ProgressStream();
const bStream = new BoundaryStream(finale);

This comment was marked as spam.

@AVaksman
Copy link
Copy Markdown
Contributor Author

I'm cool with this, but this is a very sensitive piece of code :) Please make sure before this lands that you:

  • Add a unit test to ensure the desired behavior
  • npm link this change into google-api-nodejs-client, and run through the file multipart upload samples to make sure everything still works. Once that's done - happy to approve!
  • Added unit test.
  • Ran all samples in google-api-nodejs-client with these changes, everything still works.

@JustinBeckwith JustinBeckwith added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 21, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 21, 2019
@JustinBeckwith JustinBeckwith merged commit b7a77db into googleapis:master Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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: file upload stream backpressure issue

6 participants