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

Thread-safe mmap resize #1891

Merged
merged 1 commit into from
Jan 31, 2025
Merged

Thread-safe mmap resize #1891

merged 1 commit into from
Jan 31, 2025

Conversation

BCSharp
Copy link
Member

@BCSharp BCSharp commented Jan 29, 2025

Follow-up on #1875. Originally I intended to implement a fully thread-safe mmap resize, meaning the tread would wait if there were other threads using mmap at the time. At the same time I wanted to keep the requirement that in normal use (no resize in progress), regular threads do no locking, waiting, or other expensive operations (Interlocked.CompareExchange deemed not expensive). It actually seemed to be possible, with the following schema:

  1. In normal use, regular threads just update the reference count.
  2. The exclusive tread, on entry to resize, grabs the writer lock of a ReaderWriterLockSlim
  3. The exclusive thread then sets bit Exclusive in the state. This prompts incoming regular threads to fall back on the reader lock and wait on the writer lock be released.
  4. The regular threads that were already inside mmap before the writer lock was grabbed by the exclusive thread need to exit first (ref count will be informative to determine if there are any.) This is done by setting a semaphore on which the exclusive thread waits. The last exiting regular thread raises the semaphore.

In one word — complicated. So I agree with @slozier, that in such cases it should be the responsibility of the user code to ensure that mmap is not in use on other threads when resize is called. However, if not satisfied, rather than letting user threads run rampant across an unprotected mmap, it is better to simply raise an exception at the controlled entry point, just as it is now done when the mmap is already closed. I chose EAGAIN since the resource will be available again for access after a while. In this way the problem is punted back to the user, who can decide how to handle it: from not doing anything and letting the exception propagate, to catching a well defined exception (BlockingIOError) and retrying in a loop. Because the mmap code does not need to wait (and simply throws an exception), the mechanism becomes much simpler. This is what is implemented in this PR.

I also added support for preventing resize while there are existing buffer exports, since Buffer Protocol is coming in #1866. The only thing that GetBuffer/Release need to do is to increase/decrease the ref count with MmapLocker and then atomically set bit StateBits.Exporting on the first export and reset it on the last. This will handle resize, which on entry will throw BufferException while there are existing exports.

This will also handle close, although no exception will ever be thrown. It is not needed; close will be accepted and quickly return, while the whole mmap will auto-dispose when the last buffer is released. I know that it is not the same as what CPython does (which completely prevents close while there are exports), but I don't think we should be heeding CPython in this case:

  1. Firstly, we don't have to. We already have the mechanism that allows safely closing mmap with existing exports. As I said before, closing is mandatory in well-written code, so let's make it easy too.
  2. Second, trying to achieve CPython's behaviour exactly will be very tricky because .NET GC works differently than CPython's. Calling WaitForPendingFinalizer is expensive and disruptive, and on Mono will not work anyway.
  3. The philosophy of mmap is that it lives independently. For instance, for a mmap opened over an existing file, it is possible to close the underlying file and still maintain and use the mmap (indeed, there are tests in StdLib verifying it). By the same token I'd expect it to be possible to close the underlying mmap and still maintain and use its exported buffers.
  4. Finally some people are already considering CPython's behaviour a bug (mmap resize() and close() result in BufferErrors for no reason python/cpython#115635), and while we can debate the resize case, I do think they have a point with close. If close can refuse to close, people will simply stop closing mmap atogether, relying of GC, which is not the best idea.

@BCSharp BCSharp changed the title Threas-safe mmap resize Thread-safe mmap resize Jan 29, 2025
Copy link
Contributor

@slozier slozier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it took a while to review this one. I had to think it over to try and make up my mind about the close behaviour with relation to buffers. I guess the main issue I had was the expectation that once a close call completes the resources are released. But I think this will probably work. I guess we'll see if I hit issues with the buffer protocol.

@slozier slozier merged commit 3a477d2 into IronLanguages:main Jan 31, 2025
8 checks passed
@BCSharp
Copy link
Member Author

BCSharp commented Jan 31, 2025

No problem! I'm actually glad you took the time on this one. I also gave it quite some thought. We'll see what peculiarities we hit going forward.

@BCSharp BCSharp deleted the mmap_exclusive branch January 31, 2025 00:35
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