mdickinson / bigfloat

Python wrapper for MPFR, providing high-precision floating-point arithmetic
GNU Lesser General Public License v3.0
43 stars 11 forks source link

NameError thrown when using bigfloat in multi-threaded system #78

Closed mellertson closed 4 years ago

mellertson commented 6 years ago

Hello all!

I encountered a NameError exception being thrown when using bigfloat as a context manager in a multi-threaded system. The exception is only raised when bigfloat code is executed from the 2nd or 3rd threads, but it is not raised when bigfloat code is executed in the main thread. I implemented a fix, or maybe I should say it seems to fix it in my code. I'm not sure if this is a viable solution, but figured I'd post it for your review and see what you think.

In bigfloat version 0.3.0 (as reported by pip show bigfloat), I modified some lines of code starting at line 172 in bigfloat.context.py. Forgive my use of blah as a variable name, that's my goto var name at 3 am when I'm having trouble being creative. :-)

def buildLocal(mylocal=local):
    try:
        blah = mylocal.__bigfloat_context__ is None
    except AttributeError as e:
        mylocal.__bigfloat_context__ = DefaultContext

    try:
        blah = mylocal.__context_stack__ is None
    except AttributeError as e:
        mylocal.__context_stack__ = []
    return mylocal

def getcontext(_local=local):
    """
    Return the current context.

    """
    _local = buildLocal(mylocal=_local)
    return _local.__bigfloat_context__

def setcontext(context, _local=local):
    """
    Set the current context to that given.

    Attributes provided by ``context`` override those in the current
    context.  If ``context`` doesn't specify a particular attribute,
    the attribute from the current context shows through.

    """
    _local = buildLocal(mylocal=_local)
    oldcontext = getcontext()
    _local.__bigfloat_context__ = oldcontext + context

def _pushcontext(context, _local=local):
    _local = buildLocal(mylocal=_local)
    _local.__context_stack__.append(getcontext())
    setcontext(context)

def _popcontext(_local=local):
    _local = buildLocal(mylocal=_local)
    setcontext(_local.__context_stack__.pop())

I'm happy to submit a pull-request if this code meets with your approval.

mdickinson commented 6 years ago

Thanks for the report! I'm trying to get my head around what went wrong here. Do you have a self-contained snippet of code that reproduces the issue, by any chance?

Of course, all this should be updated for PEP 550, but that's a separate issue...

EDIT: Sorry, wrong PEP. PEP 567 is the one that was actually implemented for Python 3.7.

mdickinson commented 6 years ago

A full traceback would be useful too, if you happen to have one available. And yes, a PR with a fix would be very welcome, but I'd like to make sure I understand the issue first.

mdickinson commented 6 years ago

Ah, I think I have a glimmer of understanding: if a new thread is spawned after context.py is imported, then context.py doesn't get executed in the new thread and so local.__bigfloat_context__ never gets set. So there's some additional magic required here to make things work. Hmm. I originally swiped this code from decimal.py, and now I'm wondering how it worked there.

EDIT: And here's the answer: https://github.com/python/cpython/blob/4e907d8fafde6b8df9b8b461d9a6fbbf3b8674bd/Lib/decimal.py#L447-L452

... which is pretty much what your proposed solution amounts to. So yes please to a PR, if you have time. (If not, that's fine too; let me know and I'll try to get around to a fix eventually.)