-
Notifications
You must be signed in to change notification settings - Fork 976
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
[WIP] experiment - Use SemaphoreSlim to prevent concurrent basestream access #619
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #619 +/- ##
==========================================
+ Coverage 73.35% 73.40% +0.05%
==========================================
Files 68 68
Lines 8718 8735 +17
==========================================
+ Hits 6395 6412 +17
Misses 2323 2323
Continue to review full report at Codecov.
|
{ | ||
baseStream_.Dispose(); | ||
this.baseStreamSemaphore.Wait(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this the other day and wasn't sure about it, and it still feels off - if the idea of the existing lock is to prevent the stream being disposed while being read (rather than 'just' being disposed in between reads) then that needs to be maintained, but then you might also need to avoid disposing the semaphore while another thread is using it, which just makes things more complicated.
Still not sure if the lock is doing anything other than trying to save users from their own mistakes though (preventing an explicit attempt to dispose the archive while still using it) :-(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm not even sure that that is something we should try to manage. Tried looking up when the locking was added, but it lead me to this wonderful commit with 124,411 additions and 10,927 deletions... Great.
I've read elsewhere that you don't need to dispose the semaphore if you aren't using it's wait handle property directly, but I don't know if that's something to depend on. Extra related question: Should the Async stream calls use |
Regarding
from https://devblogs.microsoft.com/dotnet/configureawait-faq/#when-should-i-use-configureawaitfalse |
Yes, just thought i'd check as it can mean adding it in many places, and #574 didn't seem to do it. |
No, but that's just my bad 😅 |
A spin off / follow up to #589 which tries to use
SemaphoreSlim
to prevent concurrent access to the base stream instead oflock()
Another followup thought after looking at the bits that lock a bit more:
The
TestArchive
function:TestLocalHeader
, which locks the stream for the duration of the operationGetInputStream
, which returns the locking PartialInputStreamZipHelperStream
to read the data descriptor, which I don't think does any lockingso might (3) be a hole where multiple things could read the stream at once?
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.