Conversation
d88d6eb to
9aa0017
Compare
9aa0017 to
597af3a
Compare
|
We are not proceeding with the pre-request destination check The issue is moving the check before the request changes behavior for calls that succeed today. The file open at write time is currently the real check, and customers can legitimately rely on the request's latency window, a directory created lazily, a file deleted lazily, or async file IO that completes while the request is in flight can all make the write succeed even though the destination looks invalid at call time. Checking up front turns those working downloads into failures. |
|
This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one. |
Motivation
getObject(request, path)and the other streaming-to-file operations check the destinationfile only after the response starts arriving. So when the file can never be written, the SDK
still sends the request and downloads part of the object before failing:
CREATE_NEWand the file already exists ->FileAlreadyExistsExceptionWRITE_TO_POSITIONand the file is missing ->NoSuchFileExceptionChecking the file needs no network call, so this should fail before any request is sent. The
current behavior wastes a request and bandwidth on a download that cannot succeed.
Affects sync and async clients, and every service with a streaming output operation, not just
S3.
Modification
Validate the destination before sending the request, on both clients. The file open at write
time remains the real check; the new check only fails early. If the file is created or deleted
between the check and the open, the open still fails exactly as it does today.
FileAsyncResponseTransformer.prepare()): validate the destination first andthrow synchronously. Completing the returned future exceptionally is not enough, because the
request pipeline sends the request regardless of that future's state. The client handler
converts the throw into a failed future, so
getObject(...)still returns a future as before.FileAsyncResponseTransformerPublisher): the delegateprepare()is now called inside a try/catch that routes the error to
handleError, so the part's futurefails instead of hanging.
ResponseTransformerhas no hook that runs before the request, so one is addedinternally:
@SdkInternalApiinterfaceValidatingResponseTransformer(extendsResponseTransformer) with a singlevoid validate(). The existing anonymous class intoFile(Path)implements it in place, so no code moves and no public API is added.BaseSyncClientHandler.execute(...)callsvalidate()before sending the request, guardedby
instanceof.SdkClientExceptioncaused by the originalFileAlreadyExistsExceptionorNoSuchFileException.AsyncResponseTransformer.toFileandResponseTransformer.toFilenow states thatthe destination is validated before the request is sent.
Testing
Backward compatibility (verified)
Ran
getObjectto an existing file against the code before and after this change, with a mockHTTP client. Same exception type and same root cause on both clients.
Async - returned future completes exceptionally, before and after:
SdkClientExceptionSdkClientExceptionFileAlreadyExistsExceptionFileAlreadyExistsExceptionSync -
getObjectthrows, before and after:SdkClientExceptionSdkClientExceptionNonRetryableException->IOException->FileAlreadyExistsExceptionFileAlreadyExistsExceptionWhat changed for callers:
NonRetryableExceptionandIOExceptionwrapper layers are gone, soFileAlreadyExistsExceptionis now the direct cause. CatchingSdkClientExceptionorsearching the cause chain works as before.
getObject; asyncstill returns a failed future.
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License