Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected increase of nesting level #9

Closed
ghost opened this issue Nov 29, 2013 · 8 comments
Closed

Unexpected increase of nesting level #9

ghost opened this issue Nov 29, 2013 · 8 comments

Comments

@ghost
Copy link

ghost commented Nov 29, 2013

Originally reported by: RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew)


(originally reported in Trac by @akruis on 2013-04-12 15:30:16)

Hi,

thanks to my colleague Michael Bauer I was able to identify the following issue:

Version: 2.7-slp

If I define the following callable class

class C(object): pass
C.__call__ = some_function

and execute c=C(); c() in a tasklet then some_function runs at nesting level 1.

If I change the definition of class C to

class C(object):
    __call__ = some_function

the nesting level does not increase.

The attached test script demonstrates this problem.
I would like to fix this issue for v2.7.4-slp.


@ghost
Copy link
Author

ghost commented Nov 29, 2013

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):


@akruis on 2013-04-12 18:02:42 said:

Obviously setting of the class attribute __call__ does not update the flag Py_TPFLAGS_HAVE_STACKLESS_CALL.
The relevant source file is Objects/typeobject.c

The function type_new() (tp_new) contains a check, that sets Py_TPFLAGS_HAVE_STACKLESS_CALL if a the attribute __call__ is present and has Py_TPFLAGS_HAVE_STACKLESS_CALL.
In case of an assignment to __call__ no such test takes place. I created a patch to fix this. The patch adds a test for stackless calls to the function update_slot() which is called by type_setattro (tp_setattro). In order to avoid duplicated code, I moved the actual test into a new static function _update_stackless_call_flag().

Could anybody please have a look at this patch.

@ghost
Copy link
Author

ghost commented Nov 29, 2013

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):


@ctismer on 2013-04-12 22:38:17 said:

I examined the patch and also the addition to the tests.
This looks pretty awesome to me and hits an uncommon use case, but is
a very sensible correction.

Do you want me to test it? Otherwise feel free to check it in!
Maybe modulo a little renaming, see comments.

cheers - chris

@ghost
Copy link
Author

ghost commented Nov 29, 2013

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):


@akruis on 2013-04-12 18:54:42 said:

What's about classic classes? Let's extend the test case.....
Classic classes are don't show this problem.

@ghost
Copy link
Author

ghost commented Nov 29, 2013

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):


@akruis on 2013-04-13 20:41:12 said:

Replying to [comment:3 ctismer]:

I examined the patch and also the addition to the tests.
This looks pretty awesome to me and hits an uncommon use case, but is
a very sensible correction.

Thanks. The use case is not so uncommon: if the unpickler creates a class, that was pickled "by value", the unpickler first creates a almost empty class object and then add methods and other attributes. This strategy reduces problems with reference cycles.

Do you want me to test it? Otherwise feel free to check it in!
I really do not understand the Stackless code completely. Therefore I don't know, if there are any side effects and I'm not sure, if I sufficiently tested the patch. That's why I ask for help. So feel free to test the patch.
One thing, that I didn't test, is removing __call__:

class C(object): def __call__(self):
       pass
del C.__call__

What happens, if Py_TPFLAGS_HAVE_STACKLESS_CALL is incorrectly set? Consider something like this:

class A(object):
    def __call__(self):
        pass
# Py_TPFLAGS_HAVE_STACKLESS_CALL is set for A

#create an instance
a=A()

# and now change A.__call__ to a method without Py_TPFLAGS_HAVE_STACKLESS_CALL
A.__call__ = some_function_without_stackless_call_support

# now call a
a()

Maybe modulo a little renaming, see comments.
Do you have any suggestions. Choosing good names is one of my blind sides, especially in the evening.

Cheers
Anselm

@ghost
Copy link
Author

ghost commented Nov 29, 2013

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):


@akruis on 2013-04-16 19:51:18 said:

Unfortunately my patch is not correct, because it does not handle subclasses correctly. After some debugging, I'm fairly confident to understand the code, but today I'm to tired to work on this problem.
Plan:

  • Enhance the test:
    • Make a super class callable and test the stackless call flag on the subclass
    • Create a test case, where the stackless call flag is incorrectly set
  • Fix the test cases

I'm currently in a JEE project at a customers site. Therefore I'll need a few days to complete this task.

@ghost
Copy link
Author

ghost commented Nov 29, 2013

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):


@ctismer on 2013-04-14 17:06:25 said:

Yes, I added a comment to your repository.
But maybe you did not see that.

Anyway, doesn't matter.

ciao - chris

@ghost
Copy link
Author

ghost commented Nov 29, 2013

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):


@akruis on 2013-04-24 16:13:26 said:

I fixed the issue for 2.7-slp. Changeset [7d4f4b6fdf06]

Additionally, I found that we can safely set the Py_TPFLAGS_HAVE_STACKLESS_CALL if the tp_call function is slot_tp_call. This is much faster and enables soft switching for some rare cases. (One of these cases was a recursion test in Lib/test/test_descr. See #20). Changeset [7513dc7259b3].

To do: port to Python3

@ghost
Copy link
Author

ghost commented Dec 6, 2013

Original comment by Anselm Kruis (Bitbucket: akruis, GitHub: akruis):


Already resolved. Commit for 3.x: 29ba4df3550e

@ghost ghost closed this as completed Sep 24, 2017
akruis pushed a commit that referenced this issue Nov 1, 2017
* Fix some deprecation warnings in Doc/conf.py
* Fix an rst error in Misc/NEWS
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

0 participants