THRIFT-4002: Make generated exception classes immutable by default #1835

elprans · 2019-07-22T21:21:37Z

Currently, the generated exception classes are not hashable under Python 3
because of the generated __eq__ method. Exception objects are generally
expected to be hashable by the Python standard library. Post-construction
mutation of an exception object seems like a very unlikely case, so enable
hashing for all exceptions by making them immutable by default. This also
adds a way to opt-out of immutability by setting the python.immutable
annotation to "false".

jeking3 · 2019-07-23T11:04:47Z

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

-    return ttype->annotations_.find("python.immutable") != ttype->annotations_.end();
+    std::map<std::string, std::string>::iterator it = ttype->annotations_.find("python.immutable");
+
+    if (it == ttype->annotations_.end()) {


I'm confused as to how ttype->annotations_ would get populated with the correct data. Below in DebugProtoTest.thrift containing MutableException, it says (python.immutable = "false"). Wouldn't that populate ttype->annotations with an entry where the key is "python.immutable" and the value is "false"? The code here assumes if an entry is found then it is immutable and does not check the value.

Also if someone is going to set an attribute like "python.immutable" to false, that would mean the default without an attribute means it is immutable. That is what the code does here. So wouldn't it be better to look for an attribute of "python.mutable" = and if it is there, it is not immutable?

The code here assumes if an entry is found then it is immutable and does not check the value.

But it does: else if (it->second == "false")

Also if someone is going to set an attribute like "python.immutable" to false, that would mean the default without an attribute means it is immutable.

Only for exceptions.

So wouldn't it be better to look for an attribute of "python.mutable"

Those annotations are already "boolean" flags, so I see no benefit of adding a reverse spelling as opposed to setting a boolean to "false"

stale · 2019-10-07T19:46:27Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

cjbarth · 2019-10-07T20:45:39Z

This is still important to me. Is there anything that I can help with to move this along?

stale · 2019-10-07T20:45:40Z

This issue is no longer stale. Thank you for your contributions.

stale · 2019-12-06T21:30:56Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

cjbarth · 2019-12-06T22:07:33Z

This is still important to me. Is there anything that I can help with to move this along?

stale · 2019-12-06T22:07:35Z

This issue is no longer stale. Thank you for your contributions.

dcelasun · 2019-12-06T23:00:52Z

LGTM, but I don't personally use the Python library, so I'll leave this open for comments from other maintainers for 24 hours before merging.

@elprans can you please rebase from master?

dcelasun · 2019-12-10T20:45:46Z

Merged as 45a9827 and b40f5c2.

dcelasun

Reverted due to failing tests https://travis-ci.org/apache/thrift/builds/623369529

cjbarth · 2019-12-11T14:02:05Z

@elprans One of these failures is very easy to fix:

./lib/py/src/Thrift.py:20:1: F401 'sys' imported but unused

The other is a little more complex because it depends on the mutability of the exceptions. So, does anyone know if this test is accurate in what it tests? I ask because it appears that the two tests (here and here) are the ones modifying the exception, not any underlying Thrift code. @dcelasun do you know anything about this?

./test_suite.py
..ERROR:root:Unexpected exception in handler
Traceback (most recent call last):
  File "/thrift/src/test/py.tornado/gen-py.tornado/ThriftTest/ThriftTest.py", line 1692, in process_testException
    yield gen.maybe_future(self._handler.testException(args.arg))
  File "./test_suite.py", line 86, in testException
    x.errorCode = 1001
  File "/thrift/src/test/py.tornado/gen-py.tornado/ThriftTest/ttypes.py", line 839, in __setattr__
    raise TypeError("can't modify immutable instance")
TypeError: can't modify immutable instance
E...ERROR:root:Exception in oneway handler
Traceback (most recent call last):
  File "/thrift/src/test/py.tornado/gen-py.tornado/ThriftTest/ThriftTest.py", line 1744, in process_testOneway
    yield gen.maybe_future(self._handler.testOneway(args.secondsToSleep))
  File "./test_suite.py", line 102, in testOneway
    raise Exception('testing exception in oneway method')
Exception: testing exception in oneway method
....
======================================================================
ERROR: test_exception (__main__.ThriftTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/tornado/testing.py", line 136, in __call__
    result = self.orig_method(*args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/tornado/testing.py", line 529, in post_coroutine
    timeout=timeout)
  File "/usr/lib/python2.7/dist-packages/tornado/ioloop.py", line 458, in run_sync
    return future_cell[0].result()
  File "/usr/lib/python2.7/dist-packages/tornado/concurrent.py", line 238, in result
    raise_exc_info(self._exc_info)
  File "/usr/lib/python2.7/dist-packages/tornado/gen.py", line 1063, in run
    yielded = self.gen.throw(*exc_info)
  File "./test_suite.py", line 211, in test_exception
    yield self.client.testException('Xception')
  File "/usr/lib/python2.7/dist-packages/tornado/gen.py", line 1055, in run
    value = future.result()
  File "/usr/lib/python2.7/dist-packages/tornado/concurrent.py", line 238, in result
    raise_exc_info(self._exc_info)
  File "<string>", line 3, in raise_exc_info
TApplicationException: Internal error```

dcelasun · 2019-12-11T14:07:00Z

@dcelasun do you know anything about this?

Unfortunately no, I'm not really familiar with the Python library. Judging by git blame, those tests are 5-7 years old and if they are no longer relevant, I'd be fine with removing them.

But first I need confirmation from someone familiar with the code.

cjbarth · 2019-12-11T14:20:11Z

@bufferoverflow It appears that you and @cpiro may have originally written these tests. Would either of you happen to know why this test modifies an exception? Or, perhaps shed light on what is being tested here (maybe we don't even need theses tests anymore)?

cpiro · 2019-12-11T19:15:15Z

As you've guessed, I ripped off that test code (and large chunks of the rest of py.tornado) from py.twisted. These tests check whether exceptions have the proper behavior; they should stay in. In both cases, it should be fine to construct the Xceptions all at once instead of with mutations, i.e.

-            x = Xception()
-            x.errorCode = 1001
-            x.message = s
-            raise x
+            raise Xception(1001, s)

The original py.twisted test didn't do this either because Xception.__init__ didn't support it historically (I don't recall) or for no good reason. Note that I struggled and failed to get master to build and the tests to run, so someone else should verify that these fixes are correct.

cjbarth · 2019-12-11T19:52:36Z

@elprans would you be in a position to update this branch with the adjustment that @cpiro suggests here for both py.twisted and py.tornado in addition to removing that unneeded import? I feel like we're really close on getting this landed :)

bufferoverflow · 2019-12-11T21:34:46Z

@bufferoverflow It appears that you and @cpiro may have originally written these tests. Would either of you happen to know why this test modifies an exception? Or, perhaps shed light on what is being tested here (maybe we don't even need theses tests anymore)?

@cjbarth I'm very sorry, no clue.

This reverts commit 3d9f50d.

…le by default"" This reverts commit 1234ddf.

cjbarth · 2019-12-12T13:38:04Z

Thank you @elprans for pushing those changes. That seems to correct the previous problems. Now there appears to be a problem with the CI build itself. It says it can't open a file that clearly exists. @dcelasun Would you happen to know what might cause this sort of error, given that the file exists and wasn't modified by this PR?

test 421
        Start 421: StalenessCheckTest
421: Test command: /cygdrive/c/Python27/python.exe "/cygdrive/c/projects/thrift/compiler/cpp/test/compiler/staleness_check.py" "/cygdrive/c/projects/build/CYGWIN/x64/compiler/cpp/bin/thrift.exe"
421: Test timeout computed to be: 300
421: C:\Python27\python.exe: can't open file '/cygdrive/c/projects/thrift/compiler/cpp/test/compiler/staleness_check.py': [Errno 2] No such file or directory
421/445 Test #421: StalenessCheckTest ...............***Failed    0.05 sec

dcelasun · 2019-12-12T13:48:40Z

Appveyor builds for thrift are highly unstable and we don't really block merges if they fail. Since Travis is green, I'll merge this shortly.

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.__stacktrace__` 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 adds a compiler flag, `py:immutible_exc` that generates all exceptions as mutable so they can be used with a context manager. When the flag is used with the `python.immutable = "true"` annotation, the exception is treated as immutable. I tested this change by generating the tutorial and comparing the outputs with and without the new flag. I also copied [tutorial.thrift](tutorial/tutorial.thrift) and added the `python.immutable` annotation to ensure that the annotation always overrides the flag. Where do I go to update documentation?

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.__stacktrace__` 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 it so only fields listed in `__dict__` or `__slots__` are considerd immutable. Other fields are considred writable. I added a test that ensure that a client raising an exception can be used in a context manager.

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.__stacktrace__` 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 adds a compiler flag, `py:immutible_exc` that generates all exceptions as mutable so they can be used with a context manager. When the flag is used with the `python.immutable = "true"` annotation, the exception is treated as immutable. I tested this change by generating the tutorial and comparing the outputs with and without the new flag. I also copied [tutorial.thrift](tutorial/tutorial.thrift) and added the `python.immutable` annotation to ensure that the annotation always overrides the flag. Where do I go to update documentation?

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. This will allow exceptions to be re-raised properly by the contextmanager in Python 3.11.

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.

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](#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.

jeking3 reviewed Jul 23, 2019

View reviewed changes

stale bot added the wontfix label Oct 7, 2019

stale bot removed the wontfix label Oct 7, 2019

stale bot added the wontfix label Dec 6, 2019

stale bot removed the wontfix label Dec 6, 2019

dcelasun added the enhancement label Dec 6, 2019

dcelasun closed this Dec 10, 2019

dcelasun reopened this Dec 10, 2019

dcelasun requested changes Dec 10, 2019

View reviewed changes

dcelasun added the blocked - do not merge label Dec 10, 2019

elprans added 2 commits December 11, 2019 16:51

Revert "Revert "Remove unnecessary TException.message hack""

dec1b44

This reverts commit 3d9f50d.

Revert "Revert "THRIFT-4002: Make generated exception classes immutab…

fa7f74e

…le by default"" This reverts commit 1234ddf.

elprans force-pushed the thrift-4002-fix branch from 4934e56 to fa7f74e Compare December 11, 2019 21:52

dcelasun merged commit 9c43962 into apache:master Dec 12, 2019

ridv mentioned this pull request Dec 13, 2019

Python 2 sunsetting in Jan 2020 apache/aurora#68

Open

fishy mentioned this pull request Jun 9, 2023

Trackstack includes __exit__ in contextlib which is not in previous versions python/cpython#92118

Closed

KTAtkinson mentioned this pull request Jun 14, 2023

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

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THRIFT-4002: Make generated exception classes immutable by default #1835

THRIFT-4002: Make generated exception classes immutable by default #1835

elprans commented Jul 22, 2019

jeking3 Jul 23, 2019

elprans Aug 8, 2019

stale bot commented Oct 7, 2019

cjbarth commented Oct 7, 2019

stale bot commented Oct 7, 2019

stale bot commented Dec 6, 2019

cjbarth commented Dec 6, 2019

stale bot commented Dec 6, 2019

dcelasun commented Dec 6, 2019

dcelasun commented Dec 10, 2019

dcelasun left a comment

cjbarth commented Dec 11, 2019

dcelasun commented Dec 11, 2019

cjbarth commented Dec 11, 2019

cpiro commented Dec 11, 2019

cjbarth commented Dec 11, 2019

bufferoverflow commented Dec 11, 2019

cjbarth commented Dec 12, 2019

dcelasun commented Dec 12, 2019

THRIFT-4002: Make generated exception classes immutable by default #1835

THRIFT-4002: Make generated exception classes immutable by default #1835

Conversation

elprans commented Jul 22, 2019

jeking3 Jul 23, 2019

Choose a reason for hiding this comment

elprans Aug 8, 2019

Choose a reason for hiding this comment

stale bot commented Oct 7, 2019

cjbarth commented Oct 7, 2019

stale bot commented Oct 7, 2019

stale bot commented Dec 6, 2019

cjbarth commented Dec 6, 2019

stale bot commented Dec 6, 2019

dcelasun commented Dec 6, 2019

dcelasun commented Dec 10, 2019

dcelasun left a comment

Choose a reason for hiding this comment

cjbarth commented Dec 11, 2019

dcelasun commented Dec 11, 2019

cjbarth commented Dec 11, 2019

cpiro commented Dec 11, 2019

cjbarth commented Dec 11, 2019

bufferoverflow commented Dec 11, 2019

cjbarth commented Dec 12, 2019

dcelasun commented Dec 12, 2019