Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix consumer is never called when extract uncompressed 0 byte entry #190

Merged
merged 7 commits into from
Dec 2, 2020
Merged

Fix consumer is never called when extract uncompressed 0 byte entry #190

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7ce8e7d
Fix consumer is never called when extract uncompressed 0 byte entry
mtgto Oct 23, 2020
4580da1
Add testExtractUncompressedEmptyFile
mtgto Oct 23, 2020
b46ab6f
Merge branch 'development' into uncompressed_zerobyte
weichsel Dec 2, 2020
6c1a70b
Convert early exit condition into guard
weichsel Dec 2, 2020
fac07e8
Move empty file test case to "Darwin-only" test suite
weichsel Dec 2, 2020
8fea5b9
Clean up test case
weichsel Dec 2, 2020
f3cbad7
Simplify test case and add explanatory comment
weichsel Dec 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Sources/ZIPFoundation/Data+Serialization.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ extension Data {
var chunkSize = readInOneChunk ? size : chunkSize
var checksum = CRC32(0)
var bytesRead = 0
if size == 0 {
try consumer(Data())
return checksum
}
while bytesRead < size {
let remainingSize = size - bytesRead
chunkSize = remainingSize < chunkSize ? remainingSize : chunkSize
Expand Down
Binary file added Tests/ZIPFoundationTests/Resources/testExtractUncompressedEmptyFile.zip
View file
Open in desktop
Binary file not shown.
18 changes: 18 additions & 0 deletions Tests/ZIPFoundationTests/ZIPFoundationReadingTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,4 +269,22 @@ extension ZIPFoundationTests {
let archive = self.archive(for: #function, mode: .update)
XCTAssert(archive.totalUnitCountForAddingItem(at: nonExistantURL) == -1)
}

func testExtractUncompressedEmptyFile() {
let expectation = XCTestExpectation(description: "Called extract consumer")
let archive = self.archive(for: #function, mode: .read)
guard let entry = archive["empty.txt"] else { XCTFail("Failed to extract entry."); return }
let progress = archive.makeProgressForReading(entry)
do {
_ = try archive.extract(entry, progress: progress) { (data) in
// Progress does not finished because Progress assumes totalUnitCount > 0
// XCTAssertFalse(progress.isFinished)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When extract 0byte entry, Progress#isFinished returns false.
I don't know it is good way or not...
https://developer.apple.com/documentation/foundation/progress

IMO, It is intuitive that Progress has 1/1 progress for extracting 0byte entry.

Copy link
Owner

@weichsel weichsel Dec 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is a good point - we currently don't handle this edge case properly.
We are using a 1/1 progress for entries of type .directory already and could also use the same approach for zero-byte files.

Since this PR handles the bug where the completion handler is not called for zero-byte files, I removed the progress related code from the test case you provided. I think the progress mis-reporting should be handled in a dedicated PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are using a 1/1 progress for entries of type .directory already and could also use the same approach for zero-byte files.

It looks good to me. thanks!

XCTAssertEqual(data.count, 0)
expectation.fulfill()
}
} catch {
XCTFail("Unexpected error while trying to extract empty file of uncompressed archive.")
}
wait(for: [expectation], timeout: 3.0)
}
}