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.

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

Update unarchive to use tar crate instead of tokio_tar #127

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

kylewlacy
Copy link
Member

Closes #103

This PR updates the unarchive recipe to use the tar crate (actively maintained, but sync-only) instead of the tokio-tar crate (async, but apparently unmaintained). This was specifically done to address the bug seen in #103, where tarfiles with long names weren't getting handled properly

I referenced #117 as inspiration, but ended up with a few different decisions in the implementation. Namely, the sync work happens in a spawn_blocking Tokio task, and uses a channel for the async parts (actually building the artifacts, which boil down to database writes). I also used tokio_util::io::SyncIoBridge so that we could keep doing async decompression (which I believe is sound and won't block the executor). This also has the advantage of not needing to buffer the tarfile into memory

One disadvantage with this implementation is that the blob creation is all synchronous (there's a new save_blob_from_reader_sync function for this purpose). And, since there's no way to block to acquire a semaphore, we have to acquire a semaphore on the permit outside the spawn_blocking section, meaning we have to keep a semaphore permit for the entire duration that the archive is unpacked. I couldn't figure out a good way around this without either buffering stuff in memory or having a separate lock specifically for synchronous code

@kylewlacy kylewlacy merged commit 9224bbc into main Sep 25, 2024
5 checks passed
@kylewlacy kylewlacy deleted the switch-to-tar-crate branch September 25, 2024 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to peel the archive from ruff
1 participant