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.