edgedb-python test fails with uvloop 0.15.0

[fantix]

• EdgeDB Version: 1.0-alpha.8+dev.5432.g931252366.d20210212
• OS Version: macOS 11.2.1 / Darwin 20.3.0

This is reproducible without edgedb/uvloop using a Cython script, which illustrates how edgedb/uvloop 0.15 run:

import contextvars


cdef class P:
    cdef object t

    def __init__(self, t):
        self.t = t

    def run(self):
        self.t.clear()
        t = self.t  # segfault: self.t is NULL


cdef class T:
    cdef:
        object _p, _run

    def __init__(self):
        self._p = P(self)
        self._run = self._p.run

    def clear(self):
        self._p = None
        self._run = None


cdef main(T tt):
    ctx = contextvars.copy_context()
    ctx.run(tt._run)


if __name__ == '__main__':
    t = T()
    main(t)

[fantix]

Potential fix:

(patches uvloop)

cdef main(T tt):
    ctx = contextvars.copy_context()
    run = tt._run
    ctx.run(run)

or (patches edgedb)

    def run(self):
        t = self.t
        t.clear()

I think patching uvloop makes more sense - as ctx.run() is a C function that doesn't hold any reference to the method to run, so we should.

[elprans]

Is this a Cython bug then?

[fantix]

No, I don't think so - if we're calling tt._run() directly, Cython could INCREF the tt._run correctly; Cython doesn't INCREF the parameter in ctx.run(tt._run) which is also correct I believe. It's more like a question if we should patch CPython context.run() to INCREF the method before invoking, which I haven't thought through the pros and cons yet.

UPDATE: looking at some CPython implementations, e.g. list.sort(key=method), I think it is assumed that the caller should own the method, while list.sort() simply invokes the method without INCREF. I'll create a PR in uvloop to fix any context.run() calls then.

[fantix]

With more debugging, this looks more like a Cython bug to me now. I'll update with more details

[elprans]

@fantix, check upstream master branch/issues too. It is likely that they've fixed this already, just haven't released.

[fantix]

TL;DR: uvloop 0.15.1 should've fixed this issue on our end. I'll create an issue in Cython to continue the discussion there.

Below is an incremental list of things I tried so far:

1. ❌ Latest Cython from the master branch is also having the same issue. Searching the recent Cython issues didn't give me very relevant information.
2. ✅ After changing the run() method signature to def run(self, *args), the segfault is gone.
3. ❌ But changing to def run(self, x, y) doesn't fix the segfault.
4. ✅ At last, I changed ctx.run(tt._run) to functools.reduce(tt._run, [1, 2]), and the segfault is gone again.

Details:

TBD

[1st1]

I think this has been fixed and can be closed.

[elprans]

Seems like asyncio in Python 3.11 has a similar issue? https://github.com/MagicStack/uvloop/runs/7822201198?check_suite_focus=true#step:7:768

[fantix]

OK so there are 2 issues here.

1. Cython Fast C-Call Issue

In Python 3.9 or lower, Cython uses a fast C-call method to call contextvars.Context.run(), which does not INCREF its arguments, while in 3.10 and above, the regular Cython __Pyx__PyObject_CallOneArg uses a tuple to store the args during the call.

This difference led to the explained issue at the very beginning in Python 3.9, and the issue was fixed in uvloop by INCREFing the args manually. Still, this is an issue in Cython anyways I think (?).

2. Python 3.11 new method call

The test in uvloop however is simulating the Cython issue with a weakref, a simplified version looks like this:

import weakref

a = None

class A:
    def __init__(self):
        self.v = 42

    def f(self):
        self = weakref.ref(self)
        global a
        a = None
        print(self().v)

a = A()
a.f()

This passes in Python 3.10 and below but fails on 3.11 because of the new Python call implementation I think, so it is not an issue in 3.11 asyncio because the self reference would still be there in asyncio code during the function call.

For now I will skip this uvloop test in 3.10 and above, and seek a fix in Cython for 3.9 and lower.