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

ZipFileWriter not compatible with some types implementing AsyncWrite #11

Closed
samvrlewis opened this issue Apr 1, 2022 · 2 comments
Closed
Labels
bug Something isn't working duplicate This issue or pull request already exists

Comments

@samvrlewis
Copy link

First of all, thanks for the great crate!!

I'm currently trying to use this crate to write a zip compressed stream to a tokio::DuplexStream. This currently fails because DuplexStream implements poll_shutdown by not allowing any more writes to the stream. This is problematic as calling close on the ZipFileWriter calls shutdown() to finalize the compression encoder for each file added to the archive, which means that writes after the first call to close all fail.

I don't think DuplexStream is wrong here to not allow writes after being shutdown, the Tokio documentation for Shutdown() on the AsyncWrite trait mentions that after shutdown succeeds, "the I/O object itself is likely no longer usable." (https://docs.rs/tokio-io/0.1.13/tokio_io/trait.AsyncWrite.html#return-value)

From what I can tell, Shutdown is called here so that the async_compression encoder can finalize the encoding. I guess this is mostly OK for that use case (though I see someone has raised an issue about adding another way of finalizing - Nullus157/async-compression#141) but in this case it seems like it will stop this library working at all.

I guess it's fortunate that the tokio::File and tokio::Cursor<Vec<u8>> implementations of shutdown just call flush (https://github.com/tokio-rs/tokio/blob/702d6dccc948b6a460caa52da4e4c03b252c5c3b/tokio/src/fs/file.rs#L702 and https://github.com/tokio-rs/tokio/blob/702d6dccc948b6a460caa52da4e4c03b252c5c3b/tokio/src/io/async_write.rs#L376) so it's not so much of an issue for those two writers (at least for now, maybe the implementations will change in the future to actually shut down?).

For the time being I've worked around this by wrapping DuplexStream to do a flush instead of a shutdown, but it's not ideal:

struct DuplexStreamWrapper(DuplexStream);
impl AsyncWrite for DuplexStreamWrapper {
    fn poll_write(
        mut self: Pin<&mut Self>,
        cx: &mut std::task::Context<'_>,
        buf: &[u8],
    ) -> std::task::Poll<Result<usize, io::Error>> {
       Pin::new(&mut self.0).poll_write(cx, buf)
    }

    fn poll_flush(mut self: Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> std::task::Poll<Result<(), io::Error>> {
        Pin::new(&mut self.0).poll_flush(cx)
    }

    fn poll_shutdown(mut self: Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> std::task::Poll<Result<(), io::Error>> {
        Pin::new(&mut self.0).poll_flush(cx)
    }
}

Would be happy to help out with a fix but thought I'd raise this before trying anything in case there's a better way of going about what I'm trying to do. I'm not sure if this problem should/could be fixed/worked-around in this crate, or if a change needs to be implemented in async_compression to allow finalizing the encoder without shutting down the writer first.

@Majored Majored added bug Something isn't working duplicate This issue or pull request already exists labels Apr 1, 2022
@Majored
Copy link
Owner

Majored commented Apr 1, 2022

(is a duplicate of #8)

I've gone ahead and committed a quick workaround as per the other issue which ignores the shutdown() call internally. This indirection obviously isn't ideal though so I definitely want to push for an additional API with async_compression as per the issue you linked above.

Thank you for looking into this nonetheless - let me know if you still run into any problems with this.

@Majored Majored closed this as completed Apr 1, 2022
@samvrlewis
Copy link
Author

Thanks for the quick resolution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

2 participants