FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: fix appendable upload finalization race condition by BenWhitehead · Pull Request #3295 · googleapis/java-storage · GitHub

/ java-storage Public

fix: fix appendable upload finalization race condition - #3295

Merged
BenWhitehead merged 3 commits into
mainfrom
fix/appendable/finalize-race
Sep 16, 2025
ConversationCommits (3)ChecksFiles changed
Merged

fix: fix appendable upload finalization race condition#3295
BenWhitehead merged 3 commits into
mainfrom
fix/appendable/finalize-race

Conversation

BenWhitehead commented Sep 16, 2025

Copy link
Copy Markdown
Collaborator

When finalizing an appendable upload depending on how quickly gcs is acking bytes, we could run into a case where finish_write: true was sent before all bytes had been enqueued.

Regression introduced in 2.57.0

fix: fix appendable upload finalization race condition
496538d 
When finalizing an appendable upload depending on how quickly
gcs is acking bytes, we could run into a case where finish_write: true
was sent before all bytes had been enqueued.

Regression introduced in 2.57.0
BenWhitehead requested a review from a team September 16, 2025 19:09
product-auto-label Bot added size: l Pull request size is large. api: storage Issues related to the googleapis/java-storage API. labels Sep 16, 2025
BenWhitehead commented Sep 16, 2025
View reviewed changes
Comment thread ...rage/src/main/java/com/google/cloud/storage/BidiAppendableUnbufferedWritableByteChannel.java
if (i < lastIdx && !shouldFlush) {
appended = stream.append(datum);
} else if (i == lastIdx && nextWriteShouldFinalize) {
} else if (i == lastIdx && remainingAfterPacking == 0 && nextWriteShouldFinalize) {

BenWhitehead Sep 16, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

These two lines are the actual fix

BenWhitehead commented Sep 16, 2025
View reviewed changes
Comment thread google-cloud-storage/src/main/java/com/google/cloud/storage/BidiUploadState.java Outdated
totalSentBytes,
confirmedBytes,
size
);

BenWhitehead Sep 16, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Adding more info to the diagnostic to make it more useful in the future.

BenWhitehead commented Sep 16, 2025
View reviewed changes
Comment thread .../src/test/java/com/google/cloud/storage/BidiAppendableUnbufferedWritableByteChannelTest.java
@Rule public final TestName testName = new TestName();

@Test
public void appendAndFinalizeOnlyPerformedIfAllBytesConsumed() throws IOException {

BenWhitehead Sep 16, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The new test to verify the fix

BenWhitehead commented Sep 16, 2025
View reviewed changes
Comment thread google-cloud-storage/src/test/java/com/google/cloud/storage/BidiUploadTest.java
}
}

private static BidiStreamingCallable<BidiWriteObjectRequest, BidiWriteObjectResponse>

BenWhitehead Sep 16, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Moved into BidiUploadTestUtils.java for sharing

danielduhh commented Sep 16, 2025

Copy link
Copy Markdown
Contributor

Was this caught somewhere? Is there a bug with context?

BenWhitehead commented Sep 16, 2025

Copy link
Copy Markdown
Collaborator Author

Was this caught somewhere? Is there a bug with context?

b/445496061

ddelgrosso1 approved these changes Sep 16, 2025
View reviewed changes

ddelgrosso1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. Sync'd with Ben earlier / was discussed in biweekly.

BenWhitehead merged commit 485be18 into main Sep 16, 2025
25 checks passed
BenWhitehead deleted the fix/appendable/finalize-race branch September 16, 2025 21:17
release-please Bot mentioned this pull request Sep 15, 2025
Merged
nidhiii-27 pushed a commit to nidhiii-27/java-storage that referenced this pull request Oct 6, 2025
fix: fix appendable upload finalization race condition (googleapis#3295)
937cbc7 
When finalizing an appendable upload depending on how quickly
gcs is acking bytes, we could run into a case where finish_write: true
was sent before all bytes had been enqueued.

Regression introduced in 2.57.0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
ddelgrosso1 ddelgrosso1 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: storage Issues related to the googleapis/java-storage API. size: l Pull request size is large.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL