Skip to content

Conversation

meet2mky
Copy link
Contributor

@meet2mky meet2mky commented Jun 24, 2025

The ChunkTransferTimeout was previously initiated before reading the media chunk via Media.Chunk(). This caused the timeout to incorrectly include the media read time, leading to premature timeouts for uploads with slow media sources (e.g., slow disk I/O or slow write from application).

This change refactors the upload logic to apply the chunkRetryDeadline timer after the chunk has been read from Media.Chunk() and chunkTransferTimeout timeout only applies to the network request portion of the chunk transfer. The context.WithTimeout is now created within the transferChunk function, immediately before the doUploadRequest call, ensuring its scope is limited to the network round trip.

Additionally, this commit introduces additional unit test to validate the fix and prevent regressions:

  • A slow media read, which should now succeed.
  • A slow network response, which correctly continues to fail with a timeout.
  • Retries work as expected within chunkRetryDeadline.

Note: Testing of storage (unit tests, emulator test, integration tests) is done and results are attached in the bug.

Intenal Bug: 427490995

@meet2mky meet2mky requested a review from a team as a code owner June 24, 2025 17:58
@codyoss codyoss requested a review from BrennaEpp June 25, 2025 13:30
@meet2mky meet2mky requested a review from BrennaEpp June 30, 2025 14:25
@quartzmo quartzmo added the api: storage Issues related to the Cloud Storage API. label Jun 30, 2025
Copy link
Contributor

@BrennaEpp BrennaEpp left a comment

Choose a reason for hiding this comment

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

Looks good, can you test these changes against the main Storage package's integration and emulator tests?

meet2mky added 2 commits July 5, 2025 14:06
…quest

The `ChunkTransferTimeout` was previously initiated before reading the media chunk via `Media.Chunk()`. This caused the timeout to incorrectly include the media read time, leading to premature timeouts for uploads with slow media sources (e.g., slow disk I/O or slow write from application).

This change refactors the upload logic to apply the timeout only to the network request portion of the chunk transfer. The `context.WithTimeout` is now created within the `transferChunk` function, immediately before the `doUploadRequest` call, ensuring its scope is limited to the network round trip.

Additionally, this commit introduces a new table-driven test, `TestChunkTransferTimeout`, to validate the fix and prevent regressions. The test uses mocks to simulate:
- A slow media read, which should now succeed.
- A slow network response, which correctly continues to fail with a timeout.

Intenal Bug: 427490995
@meet2mky meet2mky changed the title fix(gensupport): Correctly scope chunk transfer timeout to network request fix(gensupport): Correctly scope chunk retry deadline and chunk transfer timeout to network request Jul 5, 2025
@meet2mky meet2mky changed the title fix(gensupport): Correctly scope chunk retry deadline and chunk transfer timeout to network request fix(gensupport): Refactor and fix chunk upload logic for robust timeout handling. Jul 5, 2025
@meet2mky meet2mky requested a review from BrennaEpp July 5, 2025 14:20
@danielduhh danielduhh requested review from tritone and removed request for BrennaEpp July 8, 2025 06:50
@meet2mky meet2mky force-pushed the main branch 2 times, most recently from 43496af to 1ed7aaf Compare July 19, 2025 10:03
@meet2mky meet2mky requested a review from BrennaEpp July 21, 2025 05:03
@meet2mky
Copy link
Contributor Author

@tritone @BrennaEpp Testing is completed and this is ready for review now.

Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Overall logic looking pretty good to me, a few questions re: testing.

We should probably also do benchmarks to make sure there isn't a regression.

@meet2mky meet2mky requested a review from tritone July 22, 2025 04:01
codyoss
codyoss approved these changes Jul 22, 2025
@codyoss codyoss added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 22, 2025
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 22, 2025
@codyoss codyoss enabled auto-merge (squash) July 22, 2025 18:50
@codyoss codyoss added the automerge Merge the pull request once unit tests and other checks pass. label Jul 22, 2025
@codyoss codyoss merged commit 93865aa into googleapis:main Jul 22, 2025
5 checks passed
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jul 22, 2025
gcf-merge-on-green bot pushed a commit that referenced this pull request Jul 22, 2025
🤖 I have created a release *beep* *boop*
---


## [0.243.0](https://togithub.com/googleapis/google-api-go-client/compare/v0.242.0...v0.243.0) (2025-07-22)


### Features

* **all:** Auto-regenerate discovery clients ([#3233](https://togithub.com/googleapis/google-api-go-client/issues/3233)) ([a269dca](https://togithub.com/googleapis/google-api-go-client/commit/a269dca39e5315e9bfc77cc05b3dbd64af9baa25))
* **all:** Auto-regenerate discovery clients ([#3235](https://togithub.com/googleapis/google-api-go-client/issues/3235)) ([b656000](https://togithub.com/googleapis/google-api-go-client/commit/b656000d19f9627de3e2817451f82c339ce4e4bb))
* **all:** Auto-regenerate discovery clients ([#3236](https://togithub.com/googleapis/google-api-go-client/issues/3236)) ([971135a](https://togithub.com/googleapis/google-api-go-client/commit/971135a0223f558b99071a3830cd73cd9f2af2e8))
* **all:** Auto-regenerate discovery clients ([#3237](https://togithub.com/googleapis/google-api-go-client/issues/3237)) ([be7e601](https://togithub.com/googleapis/google-api-go-client/commit/be7e601cede79ed6f259689857634f59c1098060))
* **all:** Auto-regenerate discovery clients ([#3239](https://togithub.com/googleapis/google-api-go-client/issues/3239)) ([b2202ca](https://togithub.com/googleapis/google-api-go-client/commit/b2202ca5711c4fd4221afc678f93ae2b550d62d0))
* **all:** Auto-regenerate discovery clients ([#3240](https://togithub.com/googleapis/google-api-go-client/issues/3240)) ([ceceb79](https://togithub.com/googleapis/google-api-go-client/commit/ceceb79c861a39d376f1576e0c40ca557f5c3293))


### Bug Fixes

* **gensupport:** Update chunk upload logic for robust timeout handling. ([#3208](https://togithub.com/googleapis/google-api-go-client/issues/3208)) ([93865aa](https://togithub.com/googleapis/google-api-go-client/commit/93865aac32e7400c2485d2e15776764110cb4df0))

---
This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
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

Successfully merging this pull request may close these issues.

6 participants