provide __file__ to extension init function

[scoder]

BPO	13429
Nosy	@loewis, @jcea, @amauryfa, @scoder, @ericsnowcurrently
Files	ext_module_init_file_path.patch: Patch that implements a protocol between dynlib loader and module creator, similar to the existing _Py_PackageContext ext_module_reinit_context.patch: Patch that implements both protocols also for the module reinit case ext_module_init_file_path_2.patch: Updated patch that does not reset _Py_ModuleImportContext after use

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2017-07-25.06:13:22.128>
created_at = <Date 2011-11-18.15:46:57.922>
labels = ['extension-modules', 'interpreter-core', 'type-feature']
title = 'provide __file__ to extension init function'
updated_at = <Date 2017-07-25.06:13:22.125>
user = 'https://github.com/scoder'

bugs.python.org fields:

activity = <Date 2017-07-25.06:13:22.125>
actor = 'scoder'
assignee = 'none'
closed = True
closed_date = <Date 2017-07-25.06:13:22.128>
closer = 'scoder'
components = ['Extension Modules', 'Interpreter Core']
creation = <Date 2011-11-18.15:46:57.922>
creator = 'scoder'
dependencies = []
files = ['23725', '23726', '23728']
hgrepos = []
issue_num = 13429
keywords = ['patch']
message_count = 15.0
messages = ['147881', '147888', '147905', '147909', '147910', '147920', '147922', '147928', '147929', '147932', '148002', '150247', '175138', '175187', '299041']
nosy_count = 6.0
nosy_names = ['loewis', 'jcea', 'amaury.forgeotdarc', 'scoder', 'Arfrever', 'eric.snow']
pr_nums = []
priority = 'normal'
resolution = 'duplicate'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue13429'
versions = ['Python 3.5']

[scoder]

In Python modules, the top-level module code sees the __file__ variable and can use it to refer to resources in package subdirectories, for example. This is not currently possible in extension modules, because __file__ is only set after running the module init function, and the module has no way to find out its runtime location.

CPython should set __file__ directly in PyModule_Create2(), based on information provided by the shared library loader. This would let PyModule_GetFilenameObject() work immediately with the newly created module object.

The relevant python-dev thread is here:

http://mail.python.org/pipermail/python-dev/2011-November/114476.html

A patch will follow soon.

[scoder]

Here is an extension to the patch that implements the protocol also for extension module reinitialisation, so that the module creation can also set __file__ and the proper package in that case. Currently without tests (and users, I guess).

[amauryfa]

I suppose that the value of _Py_ModuleImportContext is protected by the import lock?

[scoder]

I don't know how the import lock applies here. Would it have to be protected by it? The lifetime is restricted to the call of the extension module init function, and its value is saved recursively if the init function triggers further imports.

It works exactly like the existing _Py_PackageContext variable.

[amauryfa]

But the GIL can be released in many places (e.g. a Py_DECREF), and another thread can enter the same function and update the same static variable.

[scoder]

... and the module init function could create and register a different module first, and ...

Well, yes, it's a best effort thing. It's rather unlikely that the GIL would get released in between the call to the init function and the creation of the module within that function, but sure, I don't see a reason why it could not happen.

However, it can't happen in moduleobject.c between the NULL check and the setting of the __file__ attribute, so that is safe enough to not trigger crashes.

And even if the wrong __file__ value is set during the run of the init function, it will still get overwritten (and thus fixed) afterwards. So my intuition is that code that relies on this new feature will simply have to make sure the module object creation is the first thing it does, and code that does not rely on it, well, does not rely on it.

[amauryfa]

... and the module init function could create and register a
different module first, and ...
Actually, this *does* happen, the PIL module is written this way.
And I don't agree with the "best effort" argument. If there is a slight chance that this does not work, we'll have to fix it sooner or later.

But please check this import lock thing: if _PyImport_AcquireLock and _PyImport_ReleaseLock are always called around extension module initialization, then we don't have to worry about other threads; and nested calls are already handled by the "oldimportcontext" variable in your patch.

[scoder]

I'm aware that these things happen, that's why I said it. Actually, wouldn't it rather be *correct* for __file__ to be set to the same file path for all modules that an extension module creates in its init function? That would suggest that _Py_ModuleImportContext shouldn't be set to NULL after the first assignment, but instead stay alive until it gets reset by the dynlib loader. If the loader gets invoked recursively later on, it will do the right thing by storing away the old value during the import and restoring it afterwards. So _Py_ModuleImportContext would always point to the path that contains the init function that is currently being executed.

Regarding the lock (which, I assume, is simply reentrant), it's being acquired far up when the import mechanism starts, so the dynlib loader and the init function call are protected.

Note that this does not apply to the reinit case. _PyImport_FindExtensionObject() does not acquire the lock itself (which seems correct), and it can be called directly from imp.init_builtin(), i.e. from user code. Maybe that's why the _Py_PackageContext protocol was not implemented there. That's rather unfortunate, though. I guess the reasoning is that new code that uses this new feature is expected to actually be reentrant, also in parallel, because the module it creates and works on is local to the current thread until the init function terminates. So the import lock is not strictly required here. This does complicate the __file__ feature, though, so the second ("reinit") patch won't work as is. I think the right fix for Python 4 would be to simply pass a context struct into the module init function.

On a related note, I just stumbled over this code in _PyImport_FindExtensionObject():

    else {
        if (def->m_base.m_init == NULL)
            return NULL;
        mod = def->m_base.m_init();
        if (mod == NULL)
            return NULL;
        PyDict_SetItem(PyImport_GetModuleDict(), name, mod);
        Py_DECREF(mod);
    }
    if (_PyState_AddModule(mod, def) < 0) {
        PyDict_DelItem(PyImport_GetModuleDict(), name);
        Py_DECREF(mod);
        return NULL;
    }

If PyDict_SetItem() fails, this is bound to crash. I think it would be worth looking into this mechanism a bit more.

[scoder]

Updated patch that does not reset _Py_ModuleImportContext after use.

[scoder]

Replying to myself:

I think the right fix for Python 4 would be to simply pass
a context struct into the module init function.

Actually, this doesn't have to wait for Python 4. Changing the module init function to take a parameter should be backwards compatible in C. Existing code simply wouldn't read the value from the stack, and new (or updated) code could benefit from the feature, Cython code in particular.

Here is a follow-up ticket for this more general feature:

http://bugs.python.org/issue13431

[scoder]

As MvL noted in his response to bpo-13431, simply adding a parameter to the module init function cannot safely be done before Python 4. So we are back to the idea of passing the information through to the module creation function, i.e. this very issue.

A variant of the implementation would be to store the context information in thread local storage instead of a global variable. That would work around any threading issues. However, this would not be required in the normal import case, only in the reinit case, as the import case is protected by the import lock, as we have seen. Personally, I do not consider this a good idea for the time being, since I doubt that the number of users for the reinitialisation API is currently worth caring about.

In any case, the semantics of __file__ for extension modules would basically become that __file__ refers to the last library that was loaded before calling the module init function. So all extension modules that this init function creates will inherit the same __file__. My guess is that they currently end up with no __file__ attribute at all, as the loader only sets it on the module that the init function returns. So I consider that an improvement already.

[scoder]

Any comments on the last patch?