classification
Title: Store weak references in modules_by_index
Type: resource usage Stage: resolved
Components: Interpreter Core Versions: Python 3.4
process
Status: closed Resolution: wont fix
Dependencies: Superseder:
Assigned To: Nosy List: brett.cannon, eric.snow, loewis, ncoghlan, pitrou, sbt
Priority: normal Keywords: patch

Created on 2013-08-06 20:38 by pitrou, last changed 2013-09-30 20:31 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
wr_module_state.patch pitrou, 2013-08-06 20:38 review
Messages (10)
msg194571 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-08-06 20:38
modules_by_index is a near-eternal store for extension modules. It is only collected at the end of interpreter shutdown, which is much too late for regular garbage collection. This patch proposes instead to store weak references in modules_by_index, so that extension modules can be collected in a normal way when they are removed from sys.modules.

The only gotcha is that PyState_FindModule returns a borrowed reference. With this change, it becomes really important to incref the returned reference as soon as possible.
msg194583 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-08-06 21:29
Won't that change to PyState_FindModule() break code? And is it part of the stable ABI?
msg194584 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-08-06 21:37
Theoretically, people *should* already incref the result from PyState_FindModule. On the other hand, the object currently wouldn't be lost unless something else calls PyState_RemoveModule(), which is hardly every used AFAICT.

The only saving grace is that PyState_FindModule() is py3-specific, and only used for extension modules which have a positive m_size (probably not many of them yet).

(I think this issue teaches us that borrowed ref-returning APIs are a bad idea)

Unfortunately, without this change, we also make it difficult or impossible to reclaim extension modules using the GC. At least I cannot think of another way.
msg194587 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-08-06 22:04
Sounds like it needs to be changed with a notice in What's New.
msg194595 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-08-07 01:49
It seems to me that the more appropriate change here would be to redefine PyState_FindModule as return a *new* ref rather than a borrowed ref and have it do the Py_INCREF before returning.

Code using it would then need to add an appropriate Py_DECREF. A reference leak is generally a less dangerous bug than an early free.
msg194598 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-08-07 08:02
> It seems to me that the more appropriate change here would be to
> redefine PyState_FindModule as return a *new* ref rather than a
> borrowed ref and have it do the Py_INCREF before returning.
> 
> Code using it would then need to add an appropriate Py_DECREF. A
> reference leak is generally a less dangerous bug than an early free.

I hadn't thought about that. Code must add Py_DECREF only on 3.4+, then.
msg194624 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-08-07 18:10
(and of course, with module states not being PyObjects, we have the same lifetimes issues as with Py_buffers not being PyObjects....)
msg198611 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-29 17:26
I think the "new new module initialization scheme", if it takes steam, is much more promising to solve the underlying issue. I'm inclined to close this entry as "won't fix", what do you think?
msg198622 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-09-29 18:16
Fine by me.
msg198735 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-30 20:31
Ok, rejecting my own patch because of compatibility issues.
History
Date User Action Args
2013-09-30 20:31:34pitrousetstatus: open -> closed
resolution: wont fix
messages: + msg198735

stage: patch review -> resolved
2013-09-29 18:16:57brett.cannonsetmessages: + msg198622
2013-09-29 17:26:06pitrousetmessages: + msg198611
2013-08-07 18:10:34pitrousetmessages: + msg194624
2013-08-07 08:02:30pitrousetmessages: + msg194598
2013-08-07 01:49:56ncoghlansetmessages: + msg194595
2013-08-06 22:04:17brett.cannonsetmessages: + msg194587
2013-08-06 21:37:44pitrousetmessages: + msg194584
2013-08-06 21:29:20brett.cannonsetmessages: + msg194583
2013-08-06 20:38:51pitroucreate