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

Better handle baseStreams closing themselves unexpectedly #387

Merged
merged 2 commits into from
Feb 1, 2020

Conversation

Numpsy
Copy link
Contributor

@Numpsy Numpsy commented Sep 15, 2019

Possible fix for #379, using the change to InflaterInputStream suggested there.

I'm not certain if its best to do the extra check in the inflator code or in gzipinputstream - putting it in the lower level code could effect the handling of Zip files as well, but i'm not certain if the issue can effect ZipInputStream in the same way as GZipInputStream.

I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for the SharpZipLib open source product (the "Contribution"). My Contribution is licensed under the MIT License.

@Numpsy Numpsy changed the title Rw/379 Possible fix for issue 379 Sep 15, 2019
@piksel
Copy link
Member

piksel commented Sep 24, 2019

The question is, should we fix it? The cause of this problem is using a non-standard stream source. If it solves the problem and doesn't cause a significant performance hit, I'm okay with it. It's perhaps not an uncommon scenario.

@Numpsy
Copy link
Contributor Author

Numpsy commented Sep 25, 2019

Not sure, checking if the stream can be read before reading it doesn't seem that unusual, but i'll try running the benchmarks and see if it makes a visible difference.

My first thought about this had been that you could do something inside GZipInputStream as that's the place that loops about trying to read multiple blocks and that would make it more isolated, but i haven't really tested that approach.

@Numpsy
Copy link
Contributor Author

Numpsy commented Sep 26, 2019

I didn't add any GZipInputStream tests when i added the benchmark project, but fwiw the ZipInputStream benchmark doesn't show any difference with the change.

@Numpsy
Copy link
Contributor Author

Numpsy commented Jan 30, 2020

@piksel I see you closed #379, does that mean that you decided that it isn't an issue that needs fixing?

@piksel
Copy link
Member

piksel commented Feb 1, 2020

Sorry, I closed it because the problem is with FtpDataStream not acting like a proper Stream. But the change proposed is simple enough and handling it any other way is much harder. It also seems like a pretty common use case.

@piksel piksel changed the title Possible fix for issue 379 Better handle baseStreams closing themselves unexpectedly Feb 1, 2020
@piksel piksel merged commit 803b4a2 into icsharpcode:master Feb 1, 2020
@Numpsy Numpsy deleted the rw/379 branch February 2, 2020 20:21
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.

2 participants