Skip to content

Fix locking in _imp - #2116

Merged
slozier merged 2 commits into
IronLanguages:mainfrom
slozier:lock
Oct 24, 2026
Merged

slozier merged 2 commits into
IronLanguages:mainfrom
slozier:lock

Conversation

@slozier

@slozier slozier commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

_imp.acquire_lock was only counting and not blocking, so replace it with a real lock (test_lock).

importlib._bootstrap was modified to resolve issues:

  • _SpecMethods.load on 3.4 could raise an exception on a None spec, leaving the lock acquired.
  • _get_module_lock set a callback on the weakref, which runs on the finalizer thread: on 3.4 it could delete a newer lock, and on 3.6 it took the lock, which could deadlock with gc.collect() (test_lock_gc_collect).

@BCSharp

BCSharp commented Oct 24, 2026

Copy link
Copy Markdown
Member

The code uses a private lock object. Maybe rather than using System.Object unconditionally, use System.Threading.Lock on .NET 9.0 and greater? See src/dlr/src/core/GlobalUsings.cs. I'd like to introduce this mechanism in all suitable places in IronPython as well, but we have to start somewhere.

@slozier

slozier commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Maybe rather than using System.Object unconditionally, use System.Threading.Lock on .NET 9.0 and greater

Sure, I can do that

@slozier

slozier commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

@BCSharp I didn't go with the global usings approach but used a polyfill instead. Might be worth doing the same in the DLR now that we're using polyfills there...

@BCSharp

BCSharp commented Oct 24, 2026

Copy link
Copy Markdown
Member

Might be worth doing the same in the DLR now that we're using polyfills there...

Sure, I can look into it. If I remember correctly, the polyfill at the time had some inefficiencies so I didn't take it.

@slozier
slozier merged commit 8adac0f into IronLanguages:main Oct 24, 2026
35 checks passed
@slozier
slozier deleted the lock branch October 24, 2026 15:22
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