Repository navigation
Conversation
Serializing a ResumableFileDownload with completed parts drops its resume context, so the resume continued with a range GET from the file length. Restart from the beginning instead, including when a resumed download is paused again before writing.
| Thread.sleep(5); | ||
| } | ||
| // A part is recorded as completed only after its bytes are written, so give the last one time to be recorded. | ||
| Thread.sleep(500); |
There was a problem hiding this comment.
Is there something else we could wait on here? Both to make this potentially faster and less flaky.... I've been working through trying to remove some of the sleep usage in our test suites. Maybe possible to poll progress bytestransfered or something from wiremock?
| // Held responses keep a server thread busy for their whole delay after the client gives up on them, so allow more than | ||
| // WireMock's default number of threads for the tests that hold parts. |
There was a problem hiding this comment.
What does the "Held responses keep a server thread busy for their whole delay" mean practically? I'm slightly worried about the impact on the total wall clock time of these tests. Do the tests that are holding these end up adding a big chunk to our test time?
| long length = destination.length(); | ||
| Instant fileLastModified = Instant.ofEpochMilli(destination.lastModified()); | ||
| List<Integer> completedParts = MultipartDownloadUtils.completedParts(request.getObjectRequest()); | ||
| List<Integer> completedParts = |
There was a problem hiding this comment.
Possible issue: if a resumed download is paused again before it writes anything, I think the new token can lose track of the zeros left in the file by the earlier pause. The next resume would then append to the file and report success, with part of the file still zeros.
I think it can happen like this (4 parts, 2 KB each):
- The first pause happens after parts 1 and 3 finish. The local file is part 1, then zeros at bytes 2048-4095, then part 3. The token (T1) records 6144 bytes, the local file's modified time M1, and
completedParts = [1, 3]. T1 is serialized. - While paused, something on the machine (
touch, a backup or sync tool) changes the local file's modified time to M2. The contents and the S3 object don't change. - Resuming T1 sees M1 != M2 and restarts. This looks right to me.
pause()is called before the first response arrives. HeredoPausereads the length and modified time from the local file as it is now (6144, M2).completedPartsToSaveOnPausedrops T1's parts because the file changed since T1. So the new token (T2) is6144 / M2 / [].- Resuming T2 finds nothing changed and no parts, so it sends
Range: bytes=6144-and appends. Bytes 2048-4095 stay zeros.
Can you test this scenario? I think resumeMultipartDownload_restartRePausedBeforeWritingAnything_shouldStillRestartFromFirstPart covers it with one extra line after serializedTokenWithCompletedParts(...):
destination.toFile().setLastModified(destination.toFile().lastModified() + 5_000);If it is an issue, I think the fix is: when the resumed download hasn't written anything yet, the new token copies the old token's state (length, modified time, completed parts) instead of reading the current file. The next resume then sees the same mismatch and restarts again. A missing sdkResponse on the progress snapshot looks like a usable "nothing written yet" signal, since it is set when the first response headers arrive, before any bytes are written. CrtFileDownload builds its token the same way, so I think it needs the same change.
WDYT?
Motivation and Context
With the Java-based multipart S3 async client,
S3TransferManager#downloadFiledownloads parts in parallel and writes each part at its own offset in the destination file, so parts can complete out of order. When such a download is paused, theResumableFileDownloadlists the completed parts, and the in-memory multipart resume context records them so thatresumeDownloadFilecan continue part by part.The resume context is kept in memory only and is not part of a serialized
ResumableFileDownload. This change makes a resume from a serializedResumableFileDownloadthat lists completed parts restart the download from the beginning, rather than continue from the end of the destination file. This matches how the CRT-based transfer manager already handles such a token.Modifications
ResumableRequestConverter:MultipartDownloadResumeContextdoes not record them (completedPartsMissingFromResumeContext). A token whose resume context records its completed parts still continues with part GETs for the remaining parts, unchanged.canResumeDownload(token, head, request). The existing publiccanResumeDownload(token, head)is renamedisObjectOrFileModified(with inverted result), andCrtS3TransferManageruses the new name.newDownloadFileRequest) get a new, emptyMultipartDownloadResumeContext.resumeWithRangeGetIfPossible, with no behavior change.hasUnresumableRangenow delegates tocomputeResumedRange, so the CRT-based transfer manager's restart decision and the range it requests use the same rule. No behavior change.DefaultFileDownloadandCrtFileDownload: when a resumed download is paused again before it has recorded any parts or written to the destination file, the newResumableFileDownloadkeeps the completed parts of the token being resumed (ResumableRequestConverter.completedPartsToSaveOnPause), so the next resume also restarts from the beginning.Testing
New and updated unit and functional tests (WireMock) in
s3-transfer-manager:ResumeDownloadWireMockTest:ResumableRequestConverterTest: restart for completed parts without a resume context and with an empty one; remaining-parts resume with a resume context; range resume without completed parts; the restart request uses a new resume context and replaces the file;hasUnresumableRangecases.DefaultFileDownloadTestandCrtFileDownloadTest: a pause before the resumed download writes keeps the completed parts of the token being resumed, and a pause after the file was modified does not.The new tests fail without the corresponding main-code changes. Module build:
s3-transfer-manager(583 tests) passes. A full-repomvn installwas not run.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