colesbury / nogil

Multithreaded Python without the GIL
Other
2.91k stars 107 forks source link

importing `daphne.server` fails / different order of evaluation than CPython #111

Closed kkalinowski-reef closed 1 year ago

kkalinowski-reef commented 1 year ago

Hello!

I've tried using daphne with nogil-3.9.10 on macOS 13.3.1 (arm, installed via pyenv) and on Debian 11 in docker (emulated linux/amd64, official dockerhub image), and I've came to a place where I believe that there might be something off with nogil itself. The basic repro would be:

import daphne.server

The basic problem I've found is here: https://github.com/zopefoundation/zope.interface/blob/master/src/zope/interface/interface.py#L384 It's assignment to a None, that should be replaced with a WeakRefDictionary as soon as the right side, the property, is evaluated. I know, this code looks terrible. But it works on CPython. Minimal case with the same issue below:

class T:
    def __init__(self):
        self._my_dict = None

    @property
    def my_dict(self) -> dict:
        if self._my_dict is None:
            self._my_dict = dict()
        return self._my_dict

    def operation(self, key: str, entry: int) -> None:
        self._my_dict[key] = self.my_dict.get(key, entry)

if __name__ == '__main__':
    t = T()
    t.operation('test', 1)

But that's not the only issue. When I've patched the zope library, I've encountered either sigsegv or bus error, depending on where I did the modification. This could have something to do with WeakRefDictionary that is used in this zope class, but I have no proof of that.

Am I missing something obvious here? Just in case, I've tried with PYTHONGIL=1 and it changed nothing. Is there anything else I should've try or attach to help?

colesbury commented 1 year ago

Hi @kkalinowski-reef, thanks for the bug report. I'll look into fixing this, but I might not get to it until next week.

colesbury commented 1 year ago

Hi @kkalinowski-reef, I pushed a fix for the evaluation order bug. I think the other issue (the sigsegv) was due to to an incompatible version of the cryptography package, which daphne indirectly depends on. I've uploaded a more recent compatible version of cryptography (40.0.2) for Linux that fixes the sigsegv.

Still on my TODO list:

The docker image nogil/python is updated, so you should be able to use that immediately (make sure to run docker pull nogil/python). Please let me know if you run into any other issues.

kkalinowski-reef commented 1 year ago

Thank you very much. Sadly I cannot now check it (was pushed into another task), so I'm closing this issue (as it most probably is fixed now). I'll reopen it if a need arises. Thank you for your help!