[C#] Support concurrent device dispose and async commit #423
Conversation
| @@ -445,7 +449,15 @@ protected SafeFileHandle GetOrAddHandle(int _segmentId) | |||
| { | |||
| return h; | |||
| } | |||
| return logHandles.GetOrAdd(_segmentId, segmentId => CreateHandle(segmentId)); | |||
| if (_disposed) return null; | |||
There was a problem hiding this comment.
Do the callers of GetOrAddHandle need null-value handling?
There was a problem hiding this comment.
Correct, the call sites of WriteFile and ReadFile catch the exception when reading or writing to a null handle, and issue the IO callback on the current thread itself:
catch
{
Interlocked.Decrement(ref numPending);
callback(uint.MaxValue, 0, context);
results.Enqueue(result);
}| di.Delete(recursive: true); | ||
| break; | ||
| } | ||
| catch { } |
There was a problem hiding this comment.
interesting, we still expect an exception?
Did I get it wrong that after the current fix there'll be no exceptions in DirectoryInfo.Delete() ?
There was a problem hiding this comment.
This sequence, though rare, could happen:
- Async IO thread calls
Commit, which first checks_disposedto be false Disposesets_disposedto be true and disposes the file handleCommitsees the handle is null and creates a new handle (or creates a handle for different commit file)- Main thread tries to delete directory, fails
Committhread re-checks_disposeand deletes the handle it just created (for handle reclamation correctness).
To avoid this case, we retry the directory deletion (it did not happen in the tests though). Note that I removed the WaitForCommit and the check for IsFaulted state in this testcase -- if you had that then this loop would not be needed.
Also, unrelated, we did seem to notice earlier (though not confirmed) that the OS releases the handle a bit later after the dispose is called, i.e. an immediate directory deletion does not always work. This is unclear though.
Fix #417