<p dir="ltr">Oops, had a draft from a few days ago that I was interrupted before sending. Finished editing the parts I believe are still relevant.</p>
<p dir="ltr">On 25 Aug 2013 21:56, "Stefan Behnel" <<a href="mailto:stefan_ml@behnel.de">stefan_ml@behnel.de</a>> wrote:<br>
><br>
> Nick Coghlan, 24.08.2013 23:43:<br>
> > On 25 Aug 2013 01:44, "Stefan Behnel" wrote:<br>
> >> Nick Coghlan, 24.08.2013 16:22:<br>
> >>> The new _PyImport_CreateAndExecExtensionModule function does the heavy<br>
> >>> lifting:<br>
> >>><br>
> >>> <a href="https://bitbucket.org/ncoghlan/cpython_sandbox/src/081f8f7e3ee27dc309463b48e6c67cf4880fca12/Python/importdl.c?at=new_extension_imports#cl-65">https://bitbucket.org/ncoghlan/cpython_sandbox/src/081f8f7e3ee27dc309463b48e6c67cf4880fca12/Python/importdl.c?at=new_extension_imports#cl-65</a><br>

> >>><br>
> >>> One key point to note is that it *doesn't* call<br>
> >>> _PyImport_FixupExtensionObject, which is the API that handles all the<br>
> >>> PEP 3121 per-module state stuff. Instead, the idea will be for modules<br>
> >>> that don't need additional C level state to just implement<br>
> >>> PyImportExec_NAME, while those that *do* need C level state implement<br>
> >>> PyImportCreate_NAME and return a custom object (which may or may not<br>
> >>> be a module subtype).<br>
> >><br>
> >> Is it really a common case for an extension module not to need any C level<br>
> >> state at all? I mean, this might work for very simple accelerator modules<br>
> >> with only a few stand-alone functions. But anything non-trivial will<br>
> >> almost<br>
> >> certainly have some kind of global state, cache, external library, etc.,<br>
> >> and that state is best stored at the C level for safety reasons.</p>
<p dir="ltr">In my experience, most extension authors aren't writing high performance C accelerators, they're exposing an existing C API to Python. It's the cffi use case rather than the Cython use case.</p>
<p dir="ltr">My primary experience of C extensions is with such wrapper modules, and for those, the exec portion of the new API is exactly what you want. The components of the wrapper module don't share global state, they just translate between Python and a pre-existing externally stateless C API.</p>

<p dir="ltr">For that use case, a precreated module to populate with types and functions is exactly what you want to keep things simple and stateless at the C level.</p>
<p dir="ltr">> > I'd prefer to encourage people to put that state on an exported *type*<br>
> > rather than directly in the module global state. So while I agree we need<br>
> > to *support* C level module globals, I'd prefer to provide a simpler<br>
> > alternative that avoids them.<br>
><br>
> But that has an impact on the API then. Why do you want the users of an<br>
> extension module to go through a separate object (even if it's just a<br>
> singleton, for example) instead of going through functions at the module<br>
> level? We don't currently encourage or propose this design for Python<br>
> modules either. Quite the contrary, it's extremely common for Python<br>
> modules to provide most of their functionality at the function level. And<br>
> IMHO that's a good thing.</p>
<p dir="ltr">Mutable module global state is always a recipe for obscure bugs, and not something I will ever let through code review without a really good rationale. Hidden process global state is never good, just sometimes a necessary evil.</p>

<p dir="ltr">However, keep in mind my patch is currently just the part I can implement without PEP 451 module spec objects. Once those are available, then I can implement the initial hook that supports returning a completely custom object.</p>

<p dir="ltr">> Note that even global functions usually hold state, be it in the form of<br>
> globally imported modules, global caches, constants, ...</p>
<p dir="ltr">If they can be shared safely across multiple instances of the module (e.g.  immutable constants), then these can be shared at the C level. Otherwise, a custom Python type will be needed to make them instance specific.</p>

<p dir="ltr">> > We also need the create/exec split to properly support reloading. Reload<br>
> > *must* reinitialize the object already in sys.modules instead of inserting<br>
> > a different object or it completely misses the point of reloading modules<br>
> > over deleting and reimporting them (i.e. implicitly affecting the<br>
> > references from other modules that imported the original object).<br>
><br>
> Interesting. I never thought of it that way.<br>
><br>
> I'm not sure this can be done in general. What if the module has threads<br>
> running that access the global state? In that case, reinitialising the<br>
> module object itself would almost certainly lead to a crash.</p>
<p dir="ltr">My current proposal on import-sig is to make the first hook "prepare_module", and pass in the existing object in the reload case. For the extension loader, this would be reflected in the signature of the C level hook as well, so the module could decide for itself if it supported reloading.</p>

<p dir="ltr">> And what if you do "from extmodule import some_function" in a Python<br>
> module? Then reloading couldn't replace that reference, just as for normal<br>
> Python modules. Meaning that you'd still have to keep both modules properly<br>
> alive in order to prevent crashes due to lost global state of the imported<br>
> function.<br>
><br>
> The difference to Python modules here is that in Python code, you'll get<br>
> some kind of exception if state is lost during a reload. In C code, you'll<br>
> most likely get a crash.</p>
<p dir="ltr">Agreed. This is actually my primary motivation for trying to improve the "can this be reloaded or not?" aspects of the loader API in PEP 451.</p>
<p dir="ltr">><br>
> How would you even make sure global state is properly cleaned up? Would you<br>
> call tp_clear() on the module object before re-running the init code? Or<br>
> how else would you enable the init code to do the right thing during both<br>
> the first run (where global state is uninitialised) and subsequent runs<br>
> (where global state may hold valid state and owned Python references)?</p>
<p dir="ltr">Up to the module. For Python modules, we just blindly overwrite things and let the GC sort it out.</p>
<p dir="ltr">(keep in mind existing extension modules using the existing API will still never be reloaded)</p>
<p dir="ltr">><br>
> Even tp_clear() may not be enough, because it's only meant to clean up<br>
> Python references, not C-level state. Basically, for reloading to be<br>
> correct without changing the object reference, it would have to go all the<br>
> way through tp_dealloc(), catch the object at the very end, right before it<br>
> gets freed, and then re-initialise it.<br>
><br>
> This sounds like we need some kind of indirection (as you mentioned above),<br>
> but without the API impact that a separate type implies. Simply making<br>
> modules an arbitrary extension type, as I proposed, cannot solve this.<br>
><br>
> (Actually, my intuition tells me that if it can't really be made to work<br>
> 100% for Python modules, e.g. due to the from-import case, why bother with<br>
> it for extension types?)</p>
<p dir="ltr">To fix testing the C implementation of etree using the same model we use for other extension modules (that's loading a second copy rather than reloading in place, but the problems are related).</p>
<p dir="ltr">><br>
><br>
> >>> Such modules can still support reloading (e.g.<br>
> >>> to pick up reloaded or removed module dependencies) by providing<br>
> >>> PyImportExec_NAME as well.<br>
> >>><br>
> >>> (in a PEP 451 world, this would likely be split up as two separate<br>
> >>> functions, one for create, one for exec)<br>
> >><br>
> >> Can't we just always require extension modules to implement their own<br>
> >> type?<br>
> >> Sure, it's a lot of boiler plate code, but that could be handled by a<br>
> >> simple C code generator or maybe even a copy&paste example in the docs. I<br>
> >> would like to avoid making it too easy for users in the future to get<br>
> >> anything wrong with reloading or sub-interpreters. Most people won't test<br>
> >> these things for their own code and the harder it is to make them not<br>
> >> work,<br>
> >> the more likely it is that a given set of dependencies will properly work<br>
> >> in a sub-interpreter.<br>
> >><br>
> >> If users are required to implement their own type, I think it would be<br>
> >> more<br>
> >> obvious where to put global module state, how to define functions (i.e.<br>
> >> module methods), how to handle garbage collection at the global module<br>
> >> level, etc.<br>
> ><br>
> > Take a look at the current example - everything gets stored in the module<br>
> > dict for the simple case with no C level global state.<br>
><br>
> Well, you're storing types there. And those types are your module API. I<br>
> understand that it's just an example, but I don't think it matches a common<br>
> case. As far as I can see, the types are not even interacting with each<br>
> other, let alone doing any C-level access of each other. We should try to<br>
> focus on the normal case that needs C-level state and C-level field access<br>
> of extension types. Once that's solved, we can still think about how to<br>
> make the really simple cases simpler, if it turns out that they are not<br>
> simple enough.</p>
<p dir="ltr">Our experience is very different - my perspective is that the normal case either eschews C level global state in the extension module, because it causes so many problems, or else just completely ignores subinterpreter support and proper module cleanup.</p>

<p dir="ltr">> Keeping everything in the module dict is a design that (IMHO) is too error<br>
> prone. C state should be kept safely at the C level, outside of the reach<br>
> of Python code. I don't want users of my extension module to be able to<br>
> provoke a crash by saying "extmodule._xyz = None".</p>
<p dir="ltr">So don't have global state in the *extension module*, then, keep it in the regular C/C++ modules. (And don't use the exec-only approach if you do have significant global state in the extension).</p>
<p dir="ltr">> I didn't know about PyType_FromSpec(), BTW. It looks like a nice addition<br>
> for manually written code (although useless for Cython).</p>
<p dir="ltr">This is the only way to create custom types when using the stable ABI. Can I take your observation to mean that Cython doesn't currently offer the option of limiting itself to the stable ABI?</p>
<p dir="ltr">Cheers,<br>
Nick.</p>
<p dir="ltr">><br>
> Stefan<br>
><br>
><br>
> _______________________________________________<br>
> Python-Dev mailing list<br>
> <a href="mailto:Python-Dev@python.org">Python-Dev@python.org</a><br>
> <a href="http://mail.python.org/mailman/listinfo/python-dev">http://mail.python.org/mailman/listinfo/python-dev</a><br>
> Unsubscribe: <a href="http://mail.python.org/mailman/options/python-dev/ncoghlan%40gmail.com">http://mail.python.org/mailman/options/python-dev/ncoghlan%40gmail.com</a><br>
</p>