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.

Sign up for GitHub

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

Jump to bottom

Close streams on error #1324

Merged
merged 2 commits into from
Jun 19, 2019
Merged

Close streams on error #1324

merged 2 commits into from
Jun 19, 2019

Conversation

SUPERCILEX
Copy link
Contributor

No description provided.

@SUPERCILEX SUPERCILEX requested a review from a team as a code owner June 17, 2019 05:43
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 17, 2019
@SUPERCILEX SUPERCILEX mentioned this pull request Jun 17, 2019
Closed
@SUPERCILEX
Copy link
Contributor Author

cc @ajaaym

@ajaaym
Copy link
Contributor

ajaaym commented Jun 17, 2019

@SUPERCILEX can you please describe the issue for this? And if this cant be handled in application code? This library is used by googleapis/google-cloud-java and we need to see how it affects there in terms of retries before we can merge this.

@sduskis sduskis added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 17, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 17, 2019
@SUPERCILEX
Copy link
Contributor Author

I don't understand how this could possibly be the client's responsibility. Here's the scenario:

  1. Start uploading a file. For example, with Gradle Play Publisher: https://github.com/Triple-T/gradle-play-publisher/blob/2181b5bafd32d5f7554dcbd5bfd07505e2b39be2/plugin/src/main/kotlin/com/github/triplet/gradle/play/tasks/PublishListing.kt#L228-L234
  2. The upload fails with some error code (500 for example).
  3. Now, the file will still be open and the only way to close it is to kill the process who opened it.

AFAIK, you can't go backwards in a stream so the stream will broken anyway if someone tries to continue using it.

@SUPERCILEX
Copy link
Contributor Author

@ajaaym @sduskis is there anything I can do to speed the process up?

@ajaaym
Copy link
Contributor

ajaaym commented Jun 19, 2019

@SUPERCILEX it seems ok to merge, @chingor13 can you please review?

@ajaaym ajaaym requested a review from chingor13 June 19, 2019 07:08
@chingor13
Copy link
Collaborator

Yeah, I was looking into writing a test for it. The test setup for the media upload is pretty involved.

@chingor13 chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 19, 2019
@kokoro-team kokoro-team removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 19, 2019
@chingor13 chingor13 merged commit 2c21249 into googleapis:master Jun 19, 2019
@chingor13
Copy link
Collaborator

Thank you!

@chingor13 chingor13 mentioned this pull request Jun 19, 2019
Merged
@SUPERCILEX
Copy link
Contributor Author

Awesome, thank you! 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chingor13 chingor13 chingor13 approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@SUPERCILEX @ajaaym @chingor13 @sduskis @googlebot @kokoro-team