Conversation
1d1b1ca to
b6a2334
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors error handling and status tracking during resumable uploads by augmenting exceptions with the upload URL or endpoint to make them more actionable. It also simplifies synchronization in ResumableUploadChunkCoordinator, improves error messages for stream rewinding, and adds comprehensive unit tests. The review feedback highlights two critical issues: first, removing synchronization from closePayload() introduces a potential race condition on payloadClosed, which should be resolved using an explicit lock; second, recreating exceptions in augmentWithUrl discards the original stack trace, which should be preserved by copying it to the new exception instance.
| private @Nullable IOException closePayload() { | ||
| if (payloadClosed) { | ||
| return null; | ||
| } | ||
| payloadClosed = true; |
There was a problem hiding this comment.
Removing the lock from closePayload() introduces a potential race condition and visibility issues on the payloadClosed field. Since this is performance-sensitive code, prefer using an explicit lock over the synchronized keyword to protect the shared state while ensuring thread safety and visibility.
private @Nullable IOException closePayload() {
lock.lock();
try {
if (payloadClosed) {
return null;
}
payloadClosed = true;
} finally {
lock.unlock();
}References
- In performance-sensitive code, prefer using explicit locks over the 'synchronized' keyword to protect shared state while ensuring thread safety and visibility.
| if (augmented != t) { | ||
| for (Throwable suppressed : t.getSuppressed()) { | ||
| augmented.addSuppressed(suppressed); | ||
| } | ||
| payloadClosed = true; | ||
| } |
There was a problem hiding this comment.
When recreating the exception with the augmented message, the original stack trace of t is lost because a new exception instance is constructed and its stack trace is initialized to the current thread's execution point. To preserve the original stack trace for easier debugging, we should copy the stack trace from the original exception t to the augmented exception.
| if (augmented != t) { | |
| for (Throwable suppressed : t.getSuppressed()) { | |
| augmented.addSuppressed(suppressed); | |
| } | |
| payloadClosed = true; | |
| } | |
| if (augmented != t) { | |
| augmented.setStackTrace(t.getStackTrace()); | |
| for (Throwable suppressed : t.getSuppressed()) { | |
| augmented.addSuppressed(suppressed); | |
| } | |
| } |
b6a2334 to
cff442f
Compare
…and stream requirements Improve error messages across resumable upload failure paths to provide actionable context for debugging and recovery. - In ResumableUploadChunkCoordinator, augment outgoing exception messages on terminal failures with the active upload session URL (or the endpoint if session initiation failed), ensuring callers have the session URL required for diagnostic queries and manual resume. Also include the upload session URL when reporting an incomplete upload status after transmitting the final chunk. - In RewindableStreamBuffer, clarify the exception message when a server committed offset falls below the buffer base offset by explicitly explaining that a seekable stream is required to rewind to earlier offsets. - Add comprehensive unit tests in ResumableUploadCallableImplTest and RewindableStreamBufferTest verifying that the upload URL or endpoint appears in error messages across start failures, chunk transfer failures, recovery query failures, global timeout expirations, and rewind-below-base violations.
cff442f to
7719368
Compare
|
|
1 participant
Improve error messages across resumable upload failure paths to provide actionable context for debugging and recovery.
ResumableUploadChunkCoordinator, augment outgoing exception messages on terminal failures with the active upload session URL (or the endpoint if session initiation failed), ensuring callers have the session URL required for diagnostic queries and manual resume. Also include the upload session URL when reporting an incomplete upload status after transmitting the final chunk.RewindableStreamBuffer, clarify the exception message when a server committed offset falls below the buffer base offset by explicitly explaining that a seekable stream is required to rewind to earlier offsets.ResumableUploadCallableImplTestandRewindableStreamBufferTestverifying that the upload URL or endpoint appears in error messages across start failures, chunk transfer failures, recovery query failures, global timeout expirations, and rewind-below-base violations.