Repository navigation
Conversation
![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 resumable upload error handling by removing the upload URL from RewindableStreamBuffer and centralizing exception formatting in ResumableUploadFutureImpl.toTerminalException. It also improves stream cleanup by propagating suppressed exceptions during payload closure. The review feedback highlights two important improvements: adding a null check in toTerminalException to prevent a potential NullPointerException when the input exception is null, and narrowing the caught exception type in closePayload from Throwable to Exception to avoid intercepting critical JVM errors.
95bf1aa to
461fc12
Compare
461fc12 to
7c2dd99
Compare
|
|
| } | ||
| String message = firstNonNull(t.getMessage(), t.getClass().getSimpleName()); | ||
| if (uploadSessionUrl != null) { | ||
| message = message + " (upload URL: " + uploadSessionUrl + ")"; |
There was a problem hiding this comment.
We might missed this in previous PRs. I think the upload URL contains an upload id, and anyone with the id can upload without authentication? If that's the case, I think it would be better to not include the full url in the log.
There was a problem hiding this comment.
This did come up in an earlier PR - revisiting this I think that we're required to surface the full URL. From internal cross-language design docs:
"The Client Libraries should include the upload URL as part of the errors it returns to the end-user when resumable upload-related errors occur."
and
"The upload URL can be configured (in the server configuration) to be completely arbitrary, so we cannot guarantee that we can parse the upload ID. The URL will have to be surfaced in its entirety."
This branch has not been deployed
2 participants
IOException,IllegalStateException, etc.) are now wrapped inUnknownExceptionwith the original exception asgetCause(), matching GAX's usual convention (e.g.HttpJsonApiExceptionFactory).ResumableUploadChunkCoordinatorandonTimeout();toTerminalExceptionnow appends the session URL (when available) at the future boundary.closePayload(@Nullable Throwable)now attaches anypayload.close()failure viaaddSuppressed()instead of swallowing it.