search

zopefoundation / AccessControl

Security framework for Zope.
Other
12 stars 16 forks source link

Support PURE_PYTHON instead of AC_PURE_PYTHON #120

Closed icemac closed 2 years ago

icemac commented 2 years ago

I fixed the obvious problems and disabled the tests which explicitly test the C implementation so only the actually problematic ones remain.

Locally I get the following result for these changes:

$ tox -epy39
py39 installed: zc.buildout==2.13.6
py39 run-test-pre: PYTHONHASHSEED='2895537127'
py39 run-test: commands[0] | /.../AccessControl/.tox/py39/bin/buildout -nc /.../AccessControl/buildout.cfg buildout:directory=/.../AccessControl/.tox/py39 buildout:develop=/.../AccessControl install test
Develop: '/.../AccessControl'
Updating test.
Generated script '/.../AccessControl/.tox/py39/bin/test'.
py39 run-test: commands[1] | /.../AccessControl/.tox/py39/bin/test --all -pvc
Running tests at all levels
Running zope.testrunner.layer.UnitTests tests:
  Set up zope.testrunner.layer.UnitTests in 0.000 seconds.
  Running:
    106/290 (36.6%) testPython (...ntrol.tests.testZopeGuards.TestActualPython)

Error in test testPython (AccessControl.tests.testZopeGuards.TestActualPython)
Traceback (most recent call last):
  File "/.../lib/python3.9/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/.../lib/python3.9/unittest/case.py", line 592, in run
    self._callTestMethod(testMethod)
  File "/.../lib/python3.9/unittest/case.py", line 550, in _callTestMethod
    method()
  File "/.../AccessControl/src/AccessControl/tests/testZopeGuards.py", line 756, in testPython
    exec(code, its_globals)
  File "/.../AccessControl/src/AccessControl/tests/actual_python.py", line 115, in <module>
    f6()
  File "/.../AccessControl/src/AccessControl/tests/actual_python.py", line 97, in f6
    assert getattr(C, "display", None) == getattr(C, "display")
  File "/.../AccessControl/src/AccessControl/tests/testZopeGuards.py", line 655, in __call__
    return self.func(*args, **kws)
  File "/.../AccessControl/src/AccessControl/ImplPython.py", line 767, in guarded_getattr
    aq_acquire(inst, name, aq_validate, validate)
  File "/.../eggs/cp39/Acquisition-4.8-py3.9-macosx-11.0-x86_64.egg/Acquisition/__init__.py", line 802, in aq_acquire
    return aq_acquire(ImplicitAcquisitionWrapper(obj, parent),
  File "/.../eggs/cp39/Acquisition-4.8-py3.9-macosx-11.0-x86_64.egg/Acquisition/__init__.py", line 389, in __new__
    object.__setattr__(inst, '__dict__', obj.__dict__)
TypeError: __dict__ must be set to a dictionary, not a 'mappingproxy'

    107/290 (36.9%) testPythonRealAC (...tests.testZopeGuards.TestActualPython)

Error in test testPythonRealAC (AccessControl.tests.testZopeGuards.TestActualPython)
Traceback (most recent call last):
  File "/.../lib/python3.9/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/.../lib/python3.9/unittest/case.py", line 592, in run
    self._callTestMethod(testMethod)
  File "/.../lib/python3.9/unittest/case.py", line 550, in _callTestMethod
    method()
  File "/.../AccessControl/src/AccessControl/tests/testZopeGuards.py", line 769, in testPythonRealAC
    exec(code, its_globals)
  File "/.../AccessControl/src/AccessControl/tests/actual_python.py", line 115, in <module>
    f6()
  File "/.../AccessControl/src/AccessControl/tests/actual_python.py", line 97, in f6
    assert getattr(C, "display", None) == getattr(C, "display")
  File "/.../AccessControl/src/AccessControl/ImplPython.py", line 767, in guarded_getattr
    aq_acquire(inst, name, aq_validate, validate)
  File "/.../eggs/cp39/Acquisition-4.8-py3.9-macosx-11.0-x86_64.egg/Acquisition/__init__.py", line 802, in aq_acquire
    return aq_acquire(ImplicitAcquisitionWrapper(obj, parent),
  File "/.../eggs/cp39/Acquisition-4.8-py3.9-macosx-11.0-x86_64.egg/Acquisition/__init__.py", line 389, in __new__
    object.__setattr__(inst, '__dict__', obj.__dict__)
TypeError: __dict__ must be set to a dictionary, not a 'mappingproxy'

    121/290 (41.7%) testIdentityProxy (...stZopeSecurityPolicy.Python_ZSPTests)

Failure in test testIdentityProxy (AccessControl.tests.testZopeSecurityPolicy.Python_ZSPTests)
Traceback (most recent call last):
  File "/.../lib/python3.9/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/.../lib/python3.9/unittest/case.py", line 592, in run
    self._callTestMethod(testMethod)
  File "/.../lib/python3.9/unittest/case.py", line 550, in _callTestMethod
    method()
  File "/.../AccessControl/src/AccessControl/tests/testZopeSecurityPolicy.py", line 249, in testIdentityProxy
    self.assertEqual(rc, sys.getrefcount(eo))
  File "/.../lib/python3.9/unittest/case.py", line 837, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/.../lib/python3.9/unittest/case.py", line 830, in _baseAssertEqual
    raise self.failureException(msg)
AssertionError: 3 != 12

  Ran 290 tests with 1 failures, 2 errors, 21 skipped in 0.200 seconds.
Tearing down left over layers:
  Tear down zope.testrunner.layer.UnitTests in 0.000 seconds.

Tests with errors:
   testPython (AccessControl.tests.testZopeGuards.TestActualPython)
   testPythonRealAC (AccessControl.tests.testZopeGuards.TestActualPython)

Tests with failures:
   testIdentityProxy (AccessControl.tests.testZopeSecurityPolicy.Python_ZSPTests)
ERROR: InvocationError for command /.../AccessControl/.tox/py39/bin/test --all -pvc (exited with code 1)
_________________________________________________________________________ summary __________________________________________________________________________
ERROR:   py39: commands failed

See #119 for the base of this PR.

icemac commented 2 years ago

Interestingly GHA fails in other interesting ways: https://github.com/zopefoundation/AccessControl/runs/4350600029?check_suite_focus=true

d-maurer commented 2 years ago

Michael Howitz wrote at 2021-11-28 23:43 -0800:

Interestingly GHA fails in other interesting ways: https://github.com/zopefoundation/AccessControl/runs/4350600029?check_suite_focus=true

In the "PURE_PYTHON" version, AccessControl should not import cAccessControl as it does in File "/home/runner/work/AccessControl/AccessControl/src/AccessControl/ImplPython.py", line 33, in from AccessControl.cAccessControl import _what_not_even_god_should_do

Instead, it should define _what_not_even_god_should_do itself.

The likely problem is that cAccessControl internally uses the C implementation of ExtensionClass but the expectations of that are not compatible with PURE_PYTHON.

icemac commented 2 years ago

In the "PURE_PYTHON" version, AccessControl should not import cAccessControl

I did this in https://github.com/zopefoundation/AccessControl/pull/120/commits/0005e9fecaa593cf20e1fd0ebfcdd5903bcc006f – locally it did not change anything. The failures shown in the description of this PR stay the same.

icemac commented 2 years ago

https://github.com/zopefoundation/AccessControl/runs/4364181714?check_suite_focus=true now shows the same failures I see locally using tox.

d-maurer commented 2 years ago

TypeError: dict must be set to a dictionary, not a 'mappingproxy'

This is likely a limitation of the Python implementation for Acquisition: it does (currently) not allow to call aq_acquire on a class with a filter. For a class, __dict__ is not a regular dict but a dict proxy and this lets the ImplicitAcquisitionWrapper constructor fail.

icemac commented 2 years ago

@d-maurer Thank you for the explanation. What do you suggest as next steps?

d-maurer commented 2 years ago

Michael Howitz wrote at 2021-12-1 23:19 -0800: @.*** Thank you for the explanation. What do you suggest as next steps?

I am working on this.

I have already 2 ideas to resolve the Acquisition problem.

The failing refcount test looks strange -- as if something would create cycles. I will investigate once I can reproduce the problem in my typical environment (no tox).

d-maurer commented 2 years ago

The failing refcount test looks strange -- as if something would create cycles. I will investigate once I can reproduce the problem in my typical environment (no tox).

Indeed: something seems to have created cycles involving frame objects:

(Pdb) !from gc import get_referrers
(Pdb) !gr = get_referrers
(Pdb) pp gr(eo)
[[<AccessControl.tests.testZopeSecurityPolicy.ImplictAcqObject object at 0x7f981dc28470>],
 <frame object at 0x7f981dc293e8>,
 <frame object at 0x178e9f8>,
 <frame object at 0x17f6378>,
 <frame object at 0x1826408>,
 <frame object at 0x1827138>,
 <frame object at 0x176f938>,
 <frame object at 0x1826e78>,
 <frame object at 0x182b908>,
 <frame object at 0x182b138>,
 <frame object at 0x182bc58>,
 {'eo': <AccessControl.tests.testZopeSecurityPolicy.ImplictAcqObject object at 0x7f981dc28470>,
  'get_referrers': <built-in function get_referrers>,
  'gr': <built-in function get_referrers>,
  'rc': 3,
  'self': <AccessControl.tests.testZopeSecurityPolicy.Python_ZSPTests testMethod=testIdentityProxy>}]
(Pdb) !import gc; gc.collect()
188
(Pdb) p sys.getrefcount(eo)
4
(Pdb) pp gr(eo)
[{'eo': <AccessControl.tests.testZopeSecurityPolicy.ImplictAcqObject object at 0x7f981dc28470>,
  'gc': <module 'gc' (built-in)>,
  'get_referrers': <built-in function get_referrers>,
  'gr': <built-in function get_referrers>,
  'rc': 3,
  'self': <AccessControl.tests.testZopeSecurityPolicy.Python_ZSPTests testMethod=testIdentityProxy>},
 [<AccessControl.tests.testZopeSecurityPolicy.ImplictAcqObject object at 0x7f981dc28470>],
 <frame object at 0x7f981dc293e8>]

I push a commit which calls gc.collect before refcount checks. This lets the test succeed but we might be interested to find out where the reference cycles come from.

d-maurer commented 2 years ago

I push a commit which calls gc.collect before refcount checks. This lets the test succeed but we might be interested to find out where the reference cycles come from.

I tried to locate the cycles -- but I failed. The likely reason is that pdb internally calls gc.collect; thus, when I use pdb to search for cycles, they get reclaimed and disappear.

When I remember right, then gc has an option which disables the release of cycles (instead they are placed into a gc structure for examination). With something like this, the cause of the cyclic structures might be found. But for the moment, I give up.

With the changes in this PR and https://github.com/zopefoundation/Acquisition/pull/57 the tests should pass. I will merge the Acquisition PR on Monday (unless someone objects).

I suggest again, that we harmonize PURE_PYTHON and AC_PURE_PYTHON. Other packages use the logic: use the Python implementation for either Pypy or if the PURE_PYTHON envvar is set. For AccessControl we may add an additional "or if AC_PURE_PYTHON is set" to be able to check its Python implementation together with C extensions of other packages but it should behave similar to other packages regarding the PURE_PYTHON envvar.

d-maurer commented 2 years ago

I tried to locate the cycles -- but I failed.

I have found the cause of the reference cycles --> "https://github.com/zopefoundation/Acquisition/pull/57#issuecomment-986616804".

icemac commented 2 years ago

Now only Python 3.8 on Ubuntu fails reliably, see https://github.com/zopefoundation/AccessControl/runs/4557545726?check_suite_focus=true Locally on MacOS this does not fail as on MacOS in CI.

@d-maurer Do you have any idea?

icemac commented 2 years ago

@d-maurer Do you have an idea about the Python 3.8 problem on Linux or are we forced to go with AC_PURE_PYTHON as implemented in #119?

d-maurer commented 2 years ago

@d-maurer Do you have an idea about the Python 3.8 problem on Linux or are we forced to go with AC_PURE_PYTHON as implemented in #119?

The test failure indicates that there are fewer references than expected. The huge number of references (in the 30,000) indicates that the corresponding object is extremely popular -- and therefore quite unfit to be used in a reference count test. I assume that the test failure results from an intervening garbage collection (when garbage collection is activated can depend on small variations, e.g. the Python version).

In my view, the test is buggy. It should use a new object, not one referenced from thousands of places.

If we do not want to change the test object, we can call the garbage collector manually before the getrefcount calls, to become independent from automatic garbage collector activation.

icemac commented 2 years ago

@d-maurer Thank you for looking again into this issue. I saw that another test disables gc for the test run, so I do now the same for the test case which includes the failing test.

icemac commented 2 years ago

Tests were successfully running at https://github.com/zopefoundation/AccessControl/actions/runs/1828061585. (Had to start them manually as GH had disabled the workflow.)

So I think we can prepare to merge this PR.