THRIFT-5715: Python non-user defined fields mutable with slots #2816

KTAtkinson · 2023-06-12T23:40:28Z

In Python 3.11 exceptions generated by the compiler can't be used with a
context manager because they are immutable. As of Python 3.11
contextlib.contextmanager sets exc.__traceback__ in the event that
the code in the context manager errors.

As of Thrift v0.18.1 exceptions are generated as immutable by default.
See PR#1835 for more information about why exceptions were made
immutable by default.

This change makes all non-Thrift fields mutable when slots is used without dynamic. This
will allow exceptions to be re-raised properly by the contextmanager in
Python 3.11.

With tutorial code generated from head this raises a TypeError not
an InvalidOperation:

import contextlib
import sys

sys.path.append('gen-py')
from tutorial.ttypes import InvalidOperation

@contextlib.contextmanager
def example():
    yield
    return

def main():
    with example():
        raise InvalidOperation

if __name__ == "__main__":
    main()

It raises this error:

root@b81016c354b4:/src/thrift/tutorial/py# python3 contextmanager.py 
Traceback (most recent call last):
  File "/src/thrift/tutorial/py/contextmanager.py", line 5, in <module>
    from tutorial.ttypes import InvalidOperation
ModuleNotFoundError: No module named 'tutorial.ttypes'
root@b81016c354b4:/src/thrift/tutorial/py# python3 contextmanager.py 
Traceback (most recent call last):
  File "/usr/local/lib/python3.11/contextlib.py", line 155, in __exit__
    self.gen.throw(typ, value, traceback)
  File "/src/thrift/tutorial/py/contextmanager.py", line 9, in example
    yield
  File "/src/thrift/tutorial/py/contextmanager.py", line 14, in main
    raise InvalidOperation
tutorial.ttypes.InvalidOperation: InvalidOperation(whatOp=None, why=None)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/src/thrift/tutorial/py/contextmanager.py", line 17, in <module>
    main()
  File "/src/thrift/tutorial/py/contextmanager.py", line 13, in main
    with example():
  File "/usr/local/lib/python3.11/contextlib.py", line 188, in __exit__
    exc.__traceback__ = traceback
    ^^^^^^^^^^^^^^^^^
  File "/src/thrift/tutorial/py/gen-py/tutorial/ttypes.py", line 160, in __setattr__
    raise TypeError("can't modify immutable instance")
TypeError: can't modify immutable instance

The error after this change without slots, this error still happens:

root@daf9a41282a7:/src/thrift/tutorial/py# python3 ./contextmanager.py 
Traceback (most recent call last):
  File "/src/thrift/tutorial/py/./contextmanager.py", line 17, in <module>
    main()
  File "/src/thrift/tutorial/py/./contextmanager.py", line 14, in main
    raise InvalidOperation
tutorial.ttypes.InvalidOperation: InvalidOperation(whatOp=None, why=None)
root@daf9a41282a7:/src/thrift/tutorial/py# python3 ./contextmanager.py 
Traceback (most recent call last):
  File "/usr/local/lib/python3.11/contextlib.py", line 155, in __exit__
    self.gen.throw(typ, value, traceback)
  File "/src/thrift/tutorial/py/./contextmanager.py", line 9, in example
    yield
  File "/src/thrift/tutorial/py/./contextmanager.py", line 14, in main
    raise InvalidOperation
tutorial.ttypes.InvalidOperation: InvalidOperation(whatOp=None, why=None)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/src/thrift/tutorial/py/./contextmanager.py", line 17, in <module>
    main()
  File "/src/thrift/tutorial/py/./contextmanager.py", line 13, in main
    with example():
  File "/usr/local/lib/python3.11/contextlib.py", line 188, in __exit__
    exc.__traceback__ = traceback
    ^^^^^^^^^^^^^^^^^
  File "/src/thrift/tutorial/py/gen-py/tutorial/ttypes.py", line 160, in __setattr__
    raise TypeError("can't modify immutable instance")
TypeError: can't modify immutable instance

After change with slots:

root@b81016c354b4:/src/thrift/tutorial/py# python3 contextmanager.py 
Traceback (most recent call last):
  File "/src/thrift/tutorial/py/contextmanager.py", line 17, in <module>
    main()
  File "/src/thrift/tutorial/py/contextmanager.py", line 14, in main
    raise InvalidOperation
tutorial.ttypes.InvalidOperation: InvalidOperation(whatOp=None, why=None)

There is no diff in the generated code when slots is not used.

Here is the diff between thrift at head and this branch generating with slots:

src# diff -C 5 -r tutorial/gen-py-pr/head-slots/ tutorial/gen-py-pr/dev-slots/
diff -C 5 -r tutorial/gen-py-pr/head-slots/tutorial/ttypes.py tutorial/gen-py-pr/dev-slots/tutorial/ttypes.py
*** tutorial/gen-py-pr/head-slots/tutorial/ttypes.py    Thu Jun 29 16:03:16 2023
--- tutorial/gen-py-pr/dev-slots/tutorial/ttypes.py     Thu Jun 29 16:19:11 2023
***************
*** 174,186 ****
--- 174,192 ----
      def __init__(self, whatOp=None, why=None,):
          super(InvalidOperation, self).__setattr__('whatOp', whatOp)
          super(InvalidOperation, self).__setattr__('why', why)
  
      def __setattr__(self, *args):
+         if args[0] not in self.__slots__:
+             super().__setattr__(*args)
+             return
          raise TypeError("can't modify immutable instance")
  
      def __delattr__(self, *args):
+         if args[0] not in self.__slots__:
+             super().__delattr__(*args)
+             return
          raise TypeError("can't modify immutable instance")
  
      def __hash__(self):
          return hash(self.__class__) ^ hash((self.whatOp, self.why, ))

Did you create an Apache Jira ticket? (Request account here, not required for trivial changes)
If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
Did you squash your changes to a single commit? (not required, but preferred)
Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

KTAtkinson · 2023-06-14T21:51:46Z

appvayor is currently failing, but it's also failing on all other PRs. Is this expected?

fishy · 2023-06-14T22:47:44Z

compiler/cpp/src/thrift/generate/t_py_generator.cc

@@ -105,7 +106,9 @@ class t_py_generator : public t_generator {
        }
        if( import_dynbase_.empty()) {
          import_dynbase_ = "from thrift.protocol.TBase import TBase, TFrozenBase, TExceptionBase, TFrozenExceptionBase, TTransport\n";
-        }
+        } 
+      } else if( iter->first.compare("immutable_exc") == 0) {


shouldn't this be called mutable_exc or allow_mutable_exc instead? otherwise you are making gen_immutable_exc_ to false when user pass in immutable_exc arg to the compiler.

Yea this naming is a bit confusing, I'll update it :)

fishy · 2023-06-14T22:51:05Z

appvayor is currently failing, but it's also failing on all other PRs. Is this expected?

kind of.

but I think a better fix is to limit the scope of immutability of exceptions. we only need to limit the fields used by __hash__ and __eq__ to be immutable, and allow mutations of all other fields.

KTAtkinson · 2023-06-16T19:47:02Z

I updated this so that the generated python only considers fields in the __dict__ or __slots__ field when Thrift types are immutable.

fishy

@KTAtkinson can you please also paste what the new generated python exception code look like (with and without slots)?

fishy · 2023-06-16T20:59:36Z

test/py/TestClient.py

@@ -256,6 +257,19 @@ def testMultiException(self):
        y = self.client.testMultiException('success', 'foobar')
        self.assertEqual(y.string_thing, 'foobar')

+    def testExceptionInContextManager(self):


We don't really run python 3.11 in github actions so this test doesn't really verify that it fixes the issue you are seeing right now, but it will eventually.

I think there should be some sort of test here. I originally wrote the test testing the functionality directly, assigning and deleting particular attributes. I wasn't able to write a test to ensure that user defined attributes are not editable as in some cases they are editable.

My thinking with this test case is that it will at least catch the specific failure when Thrift is updated to use Python 3.11. I did try to run this test with 3.11 but there are parts of the test script that aren't compatible with 3.11.

Thoughts on how to effectively write at test here?

fishy · 2023-06-16T21:12:44Z

compiler/cpp/src/thrift/generate/t_py_generator.cc

+    if (gen_slots_) {
+      out << indent() << "if attr not in self.__slots__:" << endl;
+    } else {
+      out << indent() << "if not self.__dict__.has_key(attr):" << endl;


this might not work as expected:

$ python3 Python 3.11.4 (main, Jun 7 2023, 10:13:09) [GCC 12.2.0] on linux Type "help", "copyright", "credits" or "license" for more information. >>> class Foo: ... _bar = None ... >>> foo = Foo() >>> foo.__dict__ {} >>> foo._bar = 1 >>> foo.__dict__ {'_bar': 1}

it seems that the __dict__ would only have the key if it's used at least once?

Currently, it looks like each use defined field is explicitly set in the __init__. I believe this would avoid this issue. Is that not correct?

Alternately I could keep track of user defined fields similar to how we do for __slots__, thoughts?

my main concern here is that after contextmanager changes the __traceback__ it will start to appear in __dict__ and then it can no longer be changed again

Long story short, __traceback__ does not appear in __dict__ from what I could track down it's because the implementation of Exception is in C and __traceback__ is not technically an official property.

Fishy and I discussed off PR and decided that it was acceptable to only support context managers (in Python 3.11) with when using slots. This means any user that runs into this issue should use slots to get around their issue.

KTAtkinson · 2023-07-06T17:51:58Z

@fishy Have time to take a look at this again?

fishy · 2023-07-06T17:56:50Z

compiler/cpp/src/thrift/generate/t_py_generator.cc

-        << endl;
+    out << indent() << "def __setattr__(self, *args):" << endl;
+    indent_up();
+    if (gen_slots_) {


please add some comments inside this if block, with link to the jira ticket, to explain why we have this check.

fishy · 2023-07-06T17:57:05Z

compiler/cpp/src/thrift/generate/t_py_generator.cc

+    out << endl;
+    out << indent() << "def __delattr__(self, *args):" << endl;
+    indent_up();
+    if (gen_slots_) {


fishy · 2023-07-06T17:58:35Z

test/py/TestClient.py

+                )
+            except Exception as e:
+                self.assertTrue(
+                    isinstance(e, TypeError),


this should be isinstance(e, TypeError) and not uses_slots instead?

or probably better split into 2 separated asserts.

fishy · 2023-07-06T18:09:09Z

it was acceptable to only support context managers (in Python 3.11) with when using slots.

also to clarify, I said it's acceptable to limit the scope of this PR to only fix the slots use-case, but that does not fully fix the issue (so we cannot close the issue with this PR merged) and we should still fix the non-slots use-case later.

fishy · 2023-07-06T18:22:47Z

please also rebase your change on top of latest master. there are 2 commits related to py compiler merged recently and I want to make sure that this does not break those changes.

fishy · 2023-07-06T22:20:22Z

Please also change the title of this PR accordingly. You are no longer adding a flag to allow mutable exceptions.

compiler/cpp/src/thrift/generate/t_py_generator.cc

fishy · 2023-07-07T21:22:38Z

compiler/cpp/src/thrift/generate/t_py_generator.cc

+    // Not user-provided fields should be editable so that the Python Standard L`ibrary can edit
+    // internal fields of std library base classes. For example, in Python 3.11 context managers
+    // edit the `__traceback__` field on Exceptions. Allowing this to work with `__slots__` is


In Python 3.11 exceptions generated by the compiler can't be used with a context manager because they are immutable. As of Python 3.11 `contextlib.contextmanager` sets `exc.__traceback__` in the event that the code in the context manager errors. As of Thrift v0.18.1 exceptions are generated as immutable by default. See [PR#1835](apache#1835) for more information about why exceptions were made immutable by default. This change makes all non-Thrift fields mutable when slots is used without dynamic. This will allow exceptions to be re-raised properly by the contextmanager in Python 3.11.

KTAtkinson changed the title ~~THRIFT-5715 Python flag to generate mutible exceptions~~ THRIFT-5715: Python flag to generate mutible exceptions Jun 12, 2023

KTAtkinson force-pushed the katie-atkinson/py-mutable-errs branch 2 times, most recently from 3d1ba71 to 6bd6ace Compare June 14, 2023 21:49

KTAtkinson changed the title ~~THRIFT-5715: Python flag to generate mutible exceptions~~ THRIFT-5715: Python flag to generate mutable exceptions Jun 14, 2023

KTAtkinson marked this pull request as ready for review June 14, 2023 21:51

fishy reviewed Jun 14, 2023

View reviewed changes

KTAtkinson force-pushed the katie-atkinson/py-mutable-errs branch 2 times, most recently from 402f505 to ec45983 Compare June 16, 2023 19:44

KTAtkinson requested a review from fishy June 16, 2023 20:43

fishy reviewed Jun 16, 2023

View reviewed changes

Jens-G added the python label Jun 21, 2023

KTAtkinson requested a review from fishy June 22, 2023 17:09

KTAtkinson force-pushed the katie-atkinson/py-mutable-errs branch from 6d12bc0 to 409b4f9 Compare June 28, 2023 22:59

fishy reviewed Jul 6, 2023

View reviewed changes

KTAtkinson changed the title ~~THRIFT-5715: Python flag to generate mutable exceptions~~ THRIFT-5715: Python non-user defined fields mutable with slots Jul 7, 2023

KTAtkinson force-pushed the katie-atkinson/py-mutable-errs branch from 409b4f9 to 906ff2a Compare July 7, 2023 21:18

KTAtkinson requested a review from fishy July 7, 2023 21:19

fishy reviewed Jul 7, 2023

View reviewed changes

KTAtkinson force-pushed the katie-atkinson/py-mutable-errs branch from 23e94f8 to d3d8fd7 Compare July 7, 2023 22:31

KTAtkinson requested a review from fishy July 7, 2023 22:32

fishy approved these changes Jul 7, 2023

View reviewed changes

fishy merged commit ff9850e into apache:master Jul 7, 2023
10 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THRIFT-5715: Python non-user defined fields mutable with slots #2816

THRIFT-5715: Python non-user defined fields mutable with slots #2816

KTAtkinson commented Jun 12, 2023 •

edited

KTAtkinson commented Jun 14, 2023

fishy Jun 14, 2023 •

edited

KTAtkinson Jun 14, 2023

fishy commented Jun 14, 2023

KTAtkinson commented Jun 16, 2023

fishy left a comment

fishy Jun 16, 2023

KTAtkinson Jun 22, 2023

fishy Jun 16, 2023

KTAtkinson Jun 22, 2023

fishy Jun 22, 2023

KTAtkinson Jun 29, 2023

KTAtkinson commented Jul 6, 2023

fishy Jul 6, 2023

fishy Jul 6, 2023

fishy Jul 6, 2023

fishy Jul 6, 2023

fishy commented Jul 6, 2023

fishy commented Jul 6, 2023

fishy commented Jul 6, 2023

fishy Jul 7, 2023

THRIFT-5715: Python non-user defined fields mutable with slots #2816

THRIFT-5715: Python non-user defined fields mutable with slots #2816

Conversation

KTAtkinson commented Jun 12, 2023 • edited

KTAtkinson commented Jun 14, 2023

fishy Jun 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishy commented Jun 14, 2023

KTAtkinson commented Jun 16, 2023

fishy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KTAtkinson commented Jul 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishy commented Jul 6, 2023

fishy commented Jul 6, 2023

fishy commented Jul 6, 2023

Choose a reason for hiding this comment

KTAtkinson commented Jun 12, 2023 •

edited

fishy Jun 14, 2023 •

edited