Regression with native descriptors from ae93dd71c378161ff3643c9a1f708f66f97f3b5d

[brettlangdon]

ae93dd7

Introduced a regression for native descriptors.

PyFunction func_descr_get is fine with the change, it treats NULL and Py_None as equal:

https://github.com/python/cpython/blob/cc48cfca7178c2131075b88b78592fed83a82a0c/Objects/funcobject.c#L1190-L1198

But there are other tp_descr_get's which fast path on NULL and skip the type checking:

• wrapperdescr_get (wrapper_descriptor): https://github.com/python/cpython/blob/cc48cfca7178c2131075b88b78592fed83a82a0c/Objects/descrobject.c#L203-L21304
• method_get (method_descriptor): hhttps://github.com/python/cpython/blob/cc48cfca7178c2131075b88b78592fed83a82a0c/Objects/descrobject.c#L137-L160
• getset_get (getset_descriptor): https://github.com/python/cpython/blob/cc48cfca7178c2131075b88b78592fed83a82a0c/Objects/descrobject.c#L183-L201
• member_get (member_descriptor): https://github.com/python/cpython/blob/cc48cfca7178c2131075b88b78592fed83a82a0c/Objects/descrobject.c#L162-L181

descr_check is what is doing type checking against Py_None and failing for these cases.

Basic reproduction example:

import wrapt


@wrapt.decorator
def passthrough(wrapped, instance, args, kwargs):
    return wrapped(*args, **kwargs)


wrapped = passthrough(str.upper)

# Simulate tp_descr_get call
wrapped.__get__(None, str)  # TypeError on 2.2.0

Short term we should revert ae93dd7 and release 2.2.1.

I am working on a PR now to add the missing test coverage which would have caught this regression.

[brettlangdon]

@GrahamDumpleton is there a regression test I can add for the problem ae93dd7 was trying to solve? I didn't see any tests added in that PR, so I am unsure what problem it was trying to fix. The commit message mostly just sounds like the goal was to align the pure Python and C implementations better?

[GrahamDumpleton]

Can you give a more practical example of where this causes a problem?

Since one would not usually call __get__() yourself I am having trouble understanding what normal use case for method binding triggered through normal Python use would have triggered this.

[GrahamDumpleton]

Am in process of reverting it now and will release an update.

[brettlangdon]

Here is another example which can trigger the problem:

import wrapt


@wrapt.decorator
def passthrough(wrapped, instance, args, kwargs):
    return wrapped(*args, **kwargs)


class MyList(list):
    append = passthrough(list.append)

print(MyList.append)

The underlying issue is that the 2.2.0 change introduced a divergence from how CPython's own descriptor protocol works. The assumption that NULL and Py_None are interchangeable holds for func_descr_get (Python functions), but not for the native C descriptor types; method_get, wrapperdescr_get, getset_get, and member_get all short-circuit on NULL. Passing Py_None through to descr_check raises TypeError. This is why the change worked correctly for the common case of wrapping Python functions but broke for native C descriptors.

[brettlangdon]

Am in process of reverting it now and will release an update.

Thanks @GrahamDumpleton, happy to keep working with you on this. Like I said I was planning to put up a PR which adds test coverage for these other tp_descr_get native functions (although a little funky like my first repro, doing manual get calls, they'd at least get to better descriptor coverage).

[GrahamDumpleton]

Already got a set of test ready to go. Just running local test across all Python versions and pushing up a 2.2.1.dev1 build for GitHub actions to do even more tests.

[brettlangdon]

oh great, I just opened #341 but we can close it if you want

[GrahamDumpleton]

Leave that PR there for now. Your tests cover more than what I had, but they are set up to only run when c extension is used, where as could be setup as necessary to still run on pure Python implementations just to ensure both variants work the same. So I'll probably merge it, pull it back into my local copy then adjust it a bit.

[GrahamDumpleton]

I have released mod_wsgi 2.2.1 that reverts that change.

After midnight for me now so time to sleep. Let me know if you find any other issues with 2.2.x. Version 2.2.0 had a large number of internal changes to address various issues with incorrect C API usage etc. Hopefully I haven't broken anything else.

[brettlangdon]

I tested wrapt 2.2.1 in our CI and everything appears to be working. We can close this issue.

Thanks for the quick response/action.

[GrahamDumpleton]

Thanks for confirming that all is good.

Given key role wrapt fulfils in an APM agent, do you run a CI pipeline which does canary testing by using pre-release candidates of wrapt (or even packages you instrument), to try and pick up issues before third party packages are released?

Knowing that changes in wrapt can have serious impacts for people doing monkey patching in particular, I deliberately push up release candidates early and leave them there for quite a while. For this release I had different release candidates up on PyPi for about a month in total and even though there were about 3 million downloads over that time, this was the first actual regression/bug in the new version which was flagged.

I know it would help me a lot if you ran tests with pre-release versions at least occasionally. Would also I am sure avoid surprises for you as well as your customers. Anyway, just a thought.