Generate full stack traces for inlineCallbacks #814

tardyp · 2017-06-12T19:06:01Z

based on #813

fixes https://twistedmatrix.com/trac/ticket/9176

first proof of concept for your review

jafd · 2017-08-11T23:23:19Z

Just in case, without this hunk

 -        return g.throw(self.type, self.value, self.tb)
 +        # note that the actual magic to find the traceback information is done in _findFailure
 +        return g.throw(self.type, self.value, None)

the one failing test suddenly passes.

I hadn't too much time to investigate why it was necessary to pass None in this one particular case, as every other case does pass self.tb wherever a traceback is needed.

I think I'll need to make a test case that would also check that the erroneous behavior on Python 3 is really gone. Do I make it into a new pull request?

rodrigc · 2017-08-11T23:27:18Z

@jafd You can make a pull request against this branch https://github.com/tardyp/twisted/tree/9176-inlinetracebacks , that way, you and @tardyp can collaborate together an move this along faster.

rodrigc · 2017-08-15T00:12:18Z

@tardyp can this PR be closed now that #813 has been updated?

jafd · 2017-08-15T10:33:08Z

Not quite sure, as this branch adds a specific test for the problem described. Would be a shame to lose it.

tardyp · 2017-08-15T12:27:40Z

Right. This branch has one commit, which improve the traceback generation for inlineCallbacks. I'll rebase them when #813 lands

rodrigc · 2017-08-17T17:36:03Z

@tardyp I updated your branch

rodrigc · 2017-08-17T17:36:31Z

@oberstet @itamarst @glyph @exarkun @moshez Please code review this if you have time.

rodrigc · 2017-08-17T18:08:41Z

src/twisted/internet/test/test_inlinecb.py

+
+    def test_forwardLotsOfTracebacks(self):
+        """
+        Test that several chained inlineCallbacks gives information about all generators


Docstring here is OK, but try to reword it so that Test that is not there, since it is obvious that it is a test. Twisted prefers to follow
docstring conventions here: https://jml.io/pages/test-docstrings.html

tardyp · 2017-09-14T19:12:57Z

@rodrigc sorry it took me so long to do this trivial change..

glyph · 2017-09-24T18:42:43Z

@tardyp Everybody's got a life to live. You don't need to apologize. Thanks for following up!

codecov · 2017-09-29T17:46:24Z

Codecov Report

Merging #814 into trunk will decrease coverage by 0.31%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##            trunk     #814      +/-   ##
==========================================
- Coverage   91.78%   91.47%   -0.32%     
==========================================
  Files         843      840       -3     
  Lines      149435   148622     -813     
  Branches    13083    12980     -103     
==========================================
- Hits       137164   135953    -1211     
- Misses      10177    10535     +358     
- Partials     2094     2134      +40

adiroiban · 2017-09-30T22:54:43Z

src/twisted/internet/test/test_inlinecb.py

+        """
+        Several Chained inlineCallbacks gives information about all generators
+
+        Reproducing #9176


the docstring for a test should not make a reference to any bugfix, but rather be self-contained and fully describe the performed action and the expected result.

Please use full sentences (with a full stop at the end) and with proper cases.

The full stop is of great help to hint whether the information is complete, or if the sentence was accidentally left unfinished.

adiroiban · 2017-09-30T23:01:08Z

src/twisted/internet/test/test_inlinecb.py

+        @inlineCallbacks
+        def erroring():
+            raise Exception()
+            yield "forcing generator"


they way this is implemented now, leaves this line without coverage.

Can you do something like

yield "forcing generator" raise Exception('some-error-marker')

In this way, the function should be converted into a generated and all the code is executed... and the marker text can be used in the assertion to make sure that the right error is raised, and this is not triggered by another error

adiroiban · 2017-09-30T23:04:36Z

@tardyp This looks pretty good.

Please look at fixing the coverage and the following small errors

src/twisted/internet/test/test_inlinecb.py (93.9%):
    149: W9012: (Class-level functions should be separated with 2 blank lines.), : Expected 2 blank lines, found 1
    153: W9401: (Used for checking comment format issues.), : Comments should begin with one whitespace

Thanks for your work!

tardyp · 2017-10-02T09:57:20Z

@adiroiban addressed comments, cleaned-up history and rebased

adiroiban

Changes look good. Thanks!

See inline comments. I don't understand why we need 2 tests.

If we really need 2, than the docstrings should be more specific about what are the diferences between the 2 tests.

I will create a copy of this branch inside twisted so that we can trigger all the tests.

Meanwhile please check the comments.

Thanks!

adiroiban · 2017-10-02T10:34:47Z

src/twisted/internet/test/test_inlinecb.py

+        Chained inlineCallbacks are forwarding the traceback informations
+        from generator to generator.
+
+        A first simple test with a couple of inline callbacks.


I don't know if this phrase add any useful info.

I think that the docstring should be

Chained inlineCallbacks are forwarding the traceback information from generator to generator and the whole chain is available as part of traceback.

adiroiban · 2017-10-02T10:36:15Z

src/twisted/internet/test/test_inlinecb.py

-        self.assertIn("in erroring", f.getTraceback())
+        tb = f.getTraceback()
+        self.assertIn("in erroring", tb)
+        self.assertIn("Error Marker", tb)


shouldn't we also check that "calling" is part of traceback?

adiroiban · 2017-10-02T10:36:53Z

src/twisted/internet/test/test_inlinecb.py

+        """
+        Several Chained inlineCallbacks gives information about all generators.
+
+        A wider test with a 4 chained inline callbacks.


what is the difference between 2 chained callbacks and 4 chained?

Do we need both tests?

I agree we don't really need two tests, but the current code passes test_forwardTracebacks, but not test_forwardLotsOfTracebacks.

So if there is a regression, this adds the info whether this is in the basic traceback generation, and/or the "advanced" stack traversal code.

Then I like to have a simple basic test, and then more tests which catch corner cases that we may find later (hence the ref to bugs). This is a methodology that we use at Buildbot, I'm perfectly okay to follow the one of Twisted, and to remove the first test, if you insist.

As you are explaining it now, we do need 2 tests:)

Please add all this information in the docstring, so that in 2 years from now, when someone will look at these tests will know why there are 2 tests without checking the whole PR and the history of those changes.

Thanks!

Right. I tried to add more details in docstrings, and in failure.py's comments

adiroiban · 2017-10-02T12:16:10Z

src/twisted/python/failure.py

@@ -252,6 +262,7 @@ def __init__(self, exc_value=None, exc_type=None, exc_tb=None,
                        tb.tb_lineno, (), ()
                        ))
                tb = tb.tb_next
+            # merging current stack with stack stored in the Failure.


I don't know if this comment is really needed... same for the above comments.

I prefer to see as little comments as possible. For me, the current code is good enough.

In 6 month we might do a change to the code and forget to update the comment ... and then the comment will just confuse rather than help.

We have the docstring for tests to explain all the use cases which this code should support

I added those comments because I did actually write the code 6 month ago, and I couldn't make sense of it at first glance, I needed a good 5 min in order to understand again why I did it like that.
Merging current stack with failure contained stack is the core idea of this trick. This is why I insist on it.
I think future change for this code needs to carefully understand it.
Please let me know which line exactly you think is superfluous.

adiroiban · 2017-10-02T12:17:07Z

src/twisted/internet/test/test_inlinecb.py

-        Test that chained inlineCallbacks are properly forwarding the traceback informations
-        from generator to generator
-        Reproducing #9175
+        Chained inlineCallbacks are forwarding the traceback informations


I have pushed a commit for fixing the informations typo, but I see that it was lost.

sorry. I fixed that.

adiroiban · 2017-10-02T12:47:47Z

I added those comments because I did actually write the code 6 month ago, and I couldn't make sense of it at first glance,

If you think that they are needed, that is fine and you can keep them.

For me is more important to have the tests and good docstring to the tests.
If someone don't understand what the merge part is doing and remove it, the test should fail... and then looking at the docstring of the test I hope that you will understand what is going on there :)

But this is not a hard requirement, so if you like the comments I am ok with keeping them.

Once tests are green I will merge them... but I think some will fail due to comment which don't start with capital letter :)

Thanks!

tardyp · 2017-10-02T13:48:41Z

For me is more important to have the tests and good docstring to the tests.
If someone don't understand what the merge part is doing and remove it, the test should fail... and then looking at the docstring of the test I hope that you will understand what is going on there :)

My reasoning was that the test docstring should explain the code external behaviour, and not the implementation details.

Once tests are green I will merge them... but I think some will fail due to comment which don't start with capital letter :)

Indeed. I fixed those.

adiroiban

All good. Thanks.

It this a PR for new feature or for fixing a defect? :)

tardyp · 2017-10-02T16:06:18Z

@adiroiban As a user of Twisted, I would have expected the tracebacks from inlineCallbacks to behave like that. This is why for me it is a very old bug.

One could argue this was never supposed to work and say it is a new feature.

adiroiban · 2017-10-03T08:08:13Z

@tardyp > As a user of Twisted, I would have expected the tracebacks from inlineCallbacks to behave like that.

I feel the same.. so maybe this should be a bugfix.

Release notes should contain the full name of the affected python object/class/module.

I have pushed a small commit, also to re-trigger appveyor, as manual trigger failed.

Now, at a second review something is strange with this PR.

The changes are in twisted.python.failure.Failure but the tests are for twisted.internet.defer.inlineCallbacks

In order to pinpoint the error, shouldn't this have at least one test in twisted.test.test_failure?

Thanks!

Before: twisted.internet.test.test_inlinecb ForwardTraceBackTests test_forwardLotsOfTracebacks ... Traceback (most recent call last): File "twisted/src/twisted/internet/defer.py", line 1532, in unwindGenerator return _inlineCallbacks(None, gen, Deferred()) File "twisted/src/twisted/internet/defer.py", line 1386, in _inlineCallbacks result = g.send(result) File "twisted/src/twisted/internet/test/test_inlinecb.py", line 162, in calling3 yield erroring() File "twisted/src/twisted/internet/defer.py", line 1532, in unwindGenerator return _inlineCallbacks(None, gen, Deferred()) --- <exception caught here> --- File "twisted/src/twisted/internet/defer.py", line 1386, in _inlineCallbacks result = g.send(result) File "twisted/src/twisted/internet/test/test_inlinecb.py", line 157, in erroring raise Exception() builtins.Exception: After: twisted.internet.test.test_inlinecb ForwardTraceBackTests test_forwardLotsOfTracebacks ... Traceback (most recent call last): File "twisted/src/twisted/internet/defer.py", line 1532, in unwindGenerator return _inlineCallbacks(None, gen, Deferred()) File "twisted/src/twisted/internet/defer.py", line 1386, in _inlineCallbacks result = g.send(result) File "twisted/src/twisted/internet/test/test_inlinecb.py", line 162, in calling3 yield erroring() File "twisted/src/twisted/internet/defer.py", line 1532, in unwindGenerator return _inlineCallbacks(None, gen, Deferred()) --- <exception caught here> --- File "twisted/src/twisted/internet/test/test_inlinecb.py", line 170, in calling yield calling2() File "twisted/src/twisted/internet/test/test_inlinecb.py", line 166, in calling2 yield calling3() File "twisted/src/twisted/internet/test/test_inlinecb.py", line 162, in calling3 yield erroring() File "twisted/src/twisted/internet/defer.py", line 1386, in _inlineCallbacks result = g.send(result) File "twisted/src/twisted/internet/test/test_inlinecb.py", line 157, in erroring raise Exception() builtins.Exception:

Actually, that unit test demonstrated an issue with the fact that original failure was changed by the failure chaining code.

tardyp · 2017-10-03T09:04:31Z

The changes are in twisted.python.failure.Failure but the tests are for twisted.internet.defer.inlineCallbacks
In order to pinpoint the error, shouldn't this have at least one test in twisted.test.test_failure?

Good point. Actually digging in those test demonstrated an issue with my code... see
cc1ad07

Fixing it adds a bit of inefficiency, though (one dict copy), but it is probably negligible compared to the stack trace fixing code.

new changes not reviewed

"The new Failure should be chained to that original Failure." Should it be the exact same Failure? or should it also include were the Failure was rethrown?

tardyp · 2017-10-03T17:03:44Z

Added some more unit test fixes. I need to change a test written in 2007 at 4fa3523

The problem is I changed the API... before the rethrown exception was exactly the same as the original exception, now it contains info about the context of where it was re-thrown.

This test has been specifically designed for inline callback. I don't see another use case of throwing a failure as an exception, so I think this make sense to change that API.

@glyph git blame shows peaker as the original writter of this code I am not sure who that is, and if we can jump him in the discussion.

hawkowl · 2018-03-31T11:27:58Z

Pushed up to buildbot to run the full test suite.

tardyp · 2018-03-31T16:40:59Z

Hi @hawkowl I fixed txchecker issue. Shall I clean the branch and rebase?

glyph · 2018-04-07T09:46:03Z

Rebase or merge trunk, either one is OK!

glyph

I have reviewed this, resolved some trivial conflicts, and it looks good to me. I think that we may want to internally do some refactoring at some point - it seems to me that this really ought to be expressed with something more akin to __cause__ and __context__, rather than treating Failure itself as an exception.

Nevertheless this is clearly more correct than what we have now.

I am going to open another PR just to get a full CI run on the merged result before landing.

tardyp force-pushed the 9176-inlinetracebacks branch 3 times, most recently from e5a0673 to ce4861a Compare August 14, 2017 16:08

rodrigc force-pushed the 9176-inlinetracebacks branch 2 times, most recently from 8e2270e to 1a1dc55 Compare August 17, 2017 17:28

rodrigc suggested changes Aug 17, 2017

View reviewed changes

tardyp force-pushed the 9176-inlinetracebacks branch from 1a1dc55 to 921664e Compare September 14, 2017 19:11

adiroiban reviewed Sep 30, 2017

View reviewed changes

tardyp force-pushed the 9176-inlinetracebacks branch from f579e0d to 7b60bf8 Compare October 2, 2017 09:56

adiroiban requested changes Oct 2, 2017

View reviewed changes

adiroiban mentioned this pull request Oct 2, 2017

[9176][twisted-branch] Generate full stack traces for inlineCallbacks. #903

Closed

tardyp force-pushed the 9176-inlinetracebacks branch from 826ff17 to 58e8784 Compare October 2, 2017 12:04

adiroiban reviewed Oct 2, 2017

View reviewed changes

adiroiban previously approved these changes Oct 2, 2017

View reviewed changes

tardyp and others added 6 commits October 3, 2017 10:59

relnote

6ff54fb

more explainations of throwExceptionIntoGenerator tricks

fc7e180

s/informations/information/

b9172ff

formatting

c0bac54

Use full name for inlineCallbacks.

88ac621

tardyp force-pushed the 9176-inlinetracebacks branch from bfd023c to 707759f Compare October 3, 2017 08:59

Add a unit test in test_failure for full traceback generation.

cc1ad07

Actually, that unit test demonstrated an issue with the fact that original failure was changed by the failure chaining code.

tardyp force-pushed the 9176-inlinetracebacks branch from 707759f to cc1ad07 Compare October 3, 2017 09:03

fix testcase for test_failure.

ba638d7

"The new Failure should be chained to that original Failure." Should it be the exact same Failure? or should it also include were the Failure was rethrown?

glyph and others added 2 commits March 26, 2018 00:11

Merge branch 'trunk' into 9176-inlinetracebacks

433b492

Merge branch 'trunk' into 9176-inlinetracebacks

42420b7

fix long lines for txchecker

5a4e12d

Merge remote-tracking branch 'twisted/trunk' into 9176-inlinetracebacks

18c72be

tardyp mentioned this pull request Apr 2, 2018

9407: fix for try/finally case in inlinecallbacks traceback generator #988

Open

4 tasks

glyph approved these changes Jun 28, 2018

View reviewed changes

glyph mentioned this pull request Jun 28, 2018

9176 redux #1031

Merged

glyph merged commit 18c72be into twisted:trunk Jun 28, 2018

tardyp deleted the 9176-inlinetracebacks branch June 28, 2018 09:13

tardyp mentioned this pull request Nov 25, 2018

tests: Fire events registered to eventual in tearDown in libvirt tests buildbot/buildbot#4440

Merged

Generate full stack traces for inlineCallbacks #814

Generate full stack traces for inlineCallbacks #814

Conversation

tardyp commented Jun 12, 2017

jafd commented Aug 11, 2017

rodrigc commented Aug 11, 2017 • edited Loading

rodrigc commented Aug 15, 2017

jafd commented Aug 15, 2017

tardyp commented Aug 15, 2017

rodrigc commented Aug 17, 2017

rodrigc commented Aug 17, 2017

Choose a reason for hiding this comment

tardyp commented Sep 14, 2017

glyph commented Sep 24, 2017

codecov bot commented Sep 29, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiroiban commented Sep 30, 2017

tardyp commented Oct 2, 2017

adiroiban 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

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

adiroiban commented Oct 2, 2017

tardyp commented Oct 2, 2017

adiroiban left a comment

Choose a reason for hiding this comment

tardyp commented Oct 2, 2017

adiroiban commented Oct 3, 2017

tardyp commented Oct 3, 2017

tardyp commented Oct 3, 2017

hawkowl commented Mar 31, 2018

tardyp commented Mar 31, 2018

glyph commented Apr 7, 2018

glyph left a comment

Choose a reason for hiding this comment

rodrigc commented Aug 11, 2017 •

edited

Loading

codecov bot commented Sep 29, 2017 •

edited

Loading