Hi everyone,
Based on the discussion at the recent core dev sprints, I’ve come up with a new revision of PEP 788. Mechanically, the proposal is nearly identical, but this time there is no such thing as an “interpreter reference”. Instead of strong and weak interpreter references, there are now interpreter locks and views, but they’re used in the same way that references were used in the old proposal.
You can read the updated text here:
Slight naming nit, PyInterpreterLock_AcquireView reads oddly to me, as it seems like it should be acquiring a view pointer from a lock pointer, rather than promoting a view pointer to a full lock pointer.
My suggestion would be to expand it to PyInterpreterLock_AcquireFromView. Alternatively, the Acquire verb could be replaced with From in both PyInterpreterLock_FromCurrent and PyInterpreterLock_FromView.
Aside from that, the updated terminology and the PEP as a whole looks good to me.
I think PyInterpreterLock_AcquireFromView is too long. Let’s go with PyInterpreterLock_FromView.
After sleeping on this for a while, I’m concerned that PyInterpreterLock may be confused with the GIL or other interpreter locking mechanisms. I think we should invent our own term instead of trying to fit into something else’s shoes.
I have a couple ideas:
PyInterpreterGuard (my personal preference)PyInterpreterShieldPyInterpreterLeaseDoes anyone have a favorite or alternatives?
I’m fine with PyInterpreterLock name with Acquire and Release verbs.
I don’t think that it can be confused with the global interpreter lock.
To CPython experts, I agree – an “interpreter lock” is not the same as a “global interpreter lock”. But I’m not so sure that will be true for users. From a user’s perspective, I could totally see someone getting tripped up by something called “Python interpreter lock” not being the GIL when they’re dealing with multithreading (and even moreso in an API designed to replace “Python GIL state”).
But even if that isn’t a problem, confusion with the GIL is only half of my concern. The other issue is that PyInterpreterLock isn’t really a lock at all, which can also be misleading to a user. For example, someone may misinterpret this as being thread-safe if called concurrently:
static int
thread(void *arg)
{
static int non_atomic = 0;
// PyInterpreterLock does not ensure mutual exclusion for this thread!
PyInterpreterLock lock = get_interpreter_lock(/* ... */);
non_atomic += 1;
PyInterpreterLock_Release(lock);
}
Yuck. What was wrong with the old terminology? “Lock” is so overloaded that it is confusing.
See the rejected ideas:
- There was contention about overcomplication in the API design; the reference counting design looked very similar to that of HPy, which had no precedent in CPython. There was fear that this proposal was being overcomplicated to look more like HPy.
- Unlike traditional reference counting APIs, acquiring a strong reference to an interpreter could arbitrarily fail, and an interpreter would not immediately deallocate when its reference count reached zero.
- There was prior discussion about adding “true” reference counting to interpreters (which would deallocate upon reaching zero), which would have been very confusing if there was an existing API in CPython titled
PyInterpreterRefthat did something different.
FWIW, I much prefer strong/weak references over locks and views. “Lock” in particular will absolutely be confused with global/per-interpreter locks.
My earlier pushback is more about having to keep a mental model of “interpreter” (thing that we didn’t create but can kind of get back to) separate from “interpreter handle” (thing that can be dup’d or closed without modifying/duplicating the actual interpreter).
“An interpreter has a reference count that you have to manage if you don’t want it to be deallocated” is a familiar mental model, and I’d really like to see us land much closer to that.
@eric.snow seems to be strongly against using a notion of references here, so I’m inclined to avoid that. I think he has a plan to expose _PyInterpreterState_IDincref/_PyInterpreterState_IDdecref publicly, which would be very confusing when paired with PyInterpreterRef.
Are you happier with PyInterpreterGuard/PyInterpreterShield like I suggested above?
I could live with Guard, but what do the APIs look like then? Which verbs do they use?
Me? Nope. The patterns I’d be happy with will either look/act like incref/decref, or the buffer protocol (i.e. PyObject_GetBuffer and PyBuffer_Free).
I’m not a fan of the ID APIs either, unless they became a GetInterpreterByToken-type API that looks pretty much like our other ByToken APIs (which are fairly new, but met the bar for “these can’t fit into any existing design”). And even then, you have to decide how to “un-get” the interpreter - either by decref or release/free.
I can work with that. How about this?
PyInterpreterGuard guard = PyInterpreterGuard_GetCurrent(); // Or perhaps NewCurrent()
/* ... */
PyInterpreterGuard_Free(guard); // Or perhaps Close() like the old API
I still think a PyInterpreterState_IncRef/_DecRef[1] and a PyInterpreterState_GetWeakRef/PyInterpreterRef_Resolve[2] makes more sense and are easier to handle. It keeps the “can we free this” state on the interpreter state directly, and only adds a new type for dealing with weak references (which is essentially the interpreter ID).
The downside of that API is only that it’s harder to debug a reference leak than it would be if each reference had clear, traceable ownership. But I don’t think that’s a compelling reason for interpreter states - they really don’t get handled all that often, and since there’s no need to increase the reference count when you’re already on a Python thread, 99% of the time it’ll never get touched. The rest of the time, it’s pretty likely that interpreter states will be created in one place(/thread) and used in another (e.g. an interpreter pool), and traceable ownership complicates that more than it helps, because there’s one obvious owner of the reference.
I agree for true interpreter reference counting, per Eric’s plan, but that’s not what this proposal is about. This proposal is about protecting the thread from being affected by interpreter finalization, but interpreter reference counting in the PyInterpreterState_IncRef way is more about owning an interpreter itself. I think we should reserve that design for the true version of reference counting, which we can probably implement after this PEP. Is that okay?
But there’s no such thing as a reference leak with PEP 788’s version of reference counting; forgetting to release a strong reference will deadlock your application.
Don’t finalize an interpreter if it has a non-zero reference count; increment the reference count when the thread would be affected.
I think the problem right now is that we try to infer “non-zero reference count” from the collection of thread states, and there are cases right now where those don’t align. Being explicit about it should give users enough control that they can solve their own problems.
Great, then there’s no objection to the IncRef API I (roughly) proposed above.
I’m trying to make the point that if PEP 788 uses reference counting to dress what is really an RW locking mechanism, it may make it very difficult to add actual reference counting (in which DecRef will immediately destroy the interpreter) later on. I too think it would be great to get public IncRef and DecRef APIs (because it would hopefully allow us to remove the notion of a “main” interpreter), but this isn’t the right proposal to do it.
I don’t think this is a requirement for “actual reference counting”. If you (as the object owner) are so aware that you are the only owner that immediate destruction matters, then you don’t need any form of reference counting. For everyone else, once you’ve said you’re not using it anymore, then it’s not important when it actually gets destroyed.
But we do already have a specific function to destroy one, so that can add a check for “exactly one reference” and do it immediately. For any code that’s currently getting it right, this will continue to work exactly the same, and code that’s getting it wrong ought to crash less (or more, if we instead make it a fatal error to explicitly destroy an interpreter that someone else is using (a.k.a. refcount > 1)).
I’m not so sure about this. Interpreter finalization is a long and expensive task, and it can occur at any DecRef call, unless we have a separate thread waiting on references, such as in this proposal. But then that means we can’t ever make DecRef calls immediately finalize, because it would almost certainly lead to deadlocks and all sorts of spurious crashes.
I really think it would be best to have two proposals:
I was interpreting it as akin to read/write locks on the shutdown machinery, but yeah, the name did give me pause as well.
I just didn’t have any better suggestions (since allusions to reference counting were rejected in the previous iteration).
Edit: and having written that, maybe keep the “view” part of the current text, but replace the “interpreter lock” part with something like “finalization guard”, with verb phrases like “block finalization” and “allow finalization” as the function names for acquiring and releasing the guard.