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

9599: Add missing attributes to fake tracebacks #1684

Merged

Conversation

pdunning-xilinx
Copy link
Contributor

@pdunning-xilinx pdunning-xilinx commented Jan 24, 2022

Scope and purpose

Real tracebacks and related objects (frames and code objects) have an extensive list of attributes. Twisted's fake tracebacks provide a cut down set of these. In Python 3, some of these attributes are used by tools such as post mortem debuggers. Thus it seems most appropriate to provide as full a set as possible (as per Python 3.10 docs), with most of them set to empty values as per the practice for some of the existing attributes.

Contributor Checklist:

  • The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/9599
  • I ran tox -e lint to format my patch to meet the Twisted Coding Standard
  • I have created a newsfragment in src/twisted/newsfragments/ (see: News files)
  • The title of the PR starts with the associated Trac ticket number (without the # character).
  • I have updated the automated tests and checked that all checks for the PR are green.
  • I have submitted the associated Trac ticket for review by adding the word review to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.
  • The merge commit will use the below format
    The first line is automatically generated by GitHub based on PR ID and branch name.
    The other lines generated by GitHub should be replaced.
Merge pull request #123 from twisted/4356-branch-name-with-trac-id

Author: pdunning-xilinx
Reviewer: <github_username>, <github_username_if_more_reviewers>
Fixes: ticket:9599

Real tracebacks and related objects (frames and code objects)
have an extensive list of attributes. Twisted's fake tracebacks
provide a cut down set of these. In Python 3, some of these
attributes are used by tools such as post mortem debuggers.
Thus it seems most appropriate to provide as full a set as possible
(as per Python 3.10 docs), with most of them set to empty values
as per the practice for some of the existing attributes.

This patch is based on #1680 to avoid merge conflict issues.

pdunning-xilinx and others added 8 commits January 4, 2022 16:42
Having f_globals and f_locals in traceback frames is useful for post
mortem debugging. Thus we should provide what we can.

If we're constructing a fake traceback then we are doing so for a
Failure that has been cleaned and therefore the variables in f_globals
and f_locals are the result of _safeReprVars(). The benefit of that is
not holding references to the original objects anymore but the downside
is that the post mortem debugger only has access to string
representations of the variables. That's still a lot better than showing
all the variables as not defined in the post mortem debugger.
Fake tracebacks can't be used by code that expects all the attributes of
a traceback if they don't have said attributes. An example of this is
post mortem debugging.
__builtins__ is not consistent accross Python implementations,
specifically Pypy 3.7. Using the builtins module is better.
@exarkun
Copy link
Member

exarkun commented Jan 24, 2022

Thanks for this contribution. The original issue reported talks about how _TracebackFrame is incompatible with certain stdlib uses. It seems like having the tests exercise these interactions would be a good thing. I believe _TracebackFrame used to be a faithful double of the real frame type - and then Python changed the frame type. If the test suite includes interop/integration tests against the users of these types in the stdlib it's more likely to catch future incompatible changes when someone is working on porting Twisted to a new Python release. If the test suite just hard-codes a list of current attributes then it won't catch such changes on its own.

@pdunning-xilinx
Copy link
Contributor Author

I could have a test that compares a fake traceback and a real one I suppose.
The problem with testing the stdlib use cases is that, if successful pdb.post_mortem() opens an interactive prompt, which will cause the test to hang. I could see if I could do the disassembly example in the ticket. I'm not aware of what other stdlib uses there are? Is there anything else that should be covered?

@exarkun
Copy link
Member

exarkun commented Jan 25, 2022

I could have a test that compares a fake traceback and a real one I suppose.

Comparing the attributes of the two types seems like it could work. It doesn't prove the fake traceback will work in all the places a real one does, but if all of the attributes are the same then the only uses I can think of that would break are uses that involve an actual type check.

The problem with testing the stdlib use cases is that, if successful pdb.post_mortem() opens an interactive prompt, which will cause the test to hang.

It might be possible to test the relevant codepath using a Pdb instance instead of the top-level function (but there is no Pdb.post_mortem, but it mostly is just a wrapper around Pdb.interact.) Pdb takes stdin and stdout args that I suppose a test could use to avoid hanging waiting for real interaction. Still, maybe this test isn't necessary if all that matters is that all the attributes are the same. It would be nice to prove it really works but I don't know what maintenance burden will come from trying to integration test against Pdb (in terms of breakage due to upstream changes to the module). We want the test to fail if there is a significant divergence between the fakes and the real implementation - not because of irrelevant incompatibilities between the pdb module and the test implementation. So, feel free to experiment, but I don't consider this mandatory.

I could see if I could do the disassembly example in the ticket.

This seems like a slightly safer test to me. It's hard to say why. Maybe because the dis module is not just an incidental implementation detail of a command line tool? It is actually meant to expose some library functionality so it should be okay for us to use it as a library in a test (and yet, only somewhat - for example, distb just dumps its output to stdout instead of returning it like a reasonable function would do). Also, what it does is a lot simpler than pdb as a whole. It might still not be a good idea to compare exact output since it seems oriented towards a human reader (so subject to arbitrary changes that someone believes will make it "easier" for a human to read) but verifying it at least doesn't raise an exception would be something.

I'm not aware of what other stdlib uses there are? Is there anything else that should be covered?

Hm. Off the top of my head, I'm not sure either. Perhaps grepping the stdlib for some of these attributes would turn up something? (Or give some confidence that there aren't really any other users)

@twm
Copy link
Contributor

twm commented Mar 5, 2022

Hi @pdunning-xilinx, I thought that this deserved mention in the release notes so I added some chattier news fragments. I also added a test that calls dis.distb() as mentioned in #9599. I hope you don't mind. I think that this sufficiently addresses @exarkun's concerns.

I have reviewed your changes against the stdlib docs and I think they're 👍. We'll need someone else to review what I did. That can be you! If my changes are okay, please comment here and I can merge. Otherwise we'll have to wait for another reviewer to happen along. I hope I'm not stepping on your toes.

Thanks for contributing to Twisted!

@twm twm requested a review from a team March 5, 2022 22:13
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Many thanks Tom for the follow up.

I have updated the ticket and PR for https://twistedmatrix.com/trac/ticket/7796 as it looks like a duplicate.

Without an explicit test for Sentry, there is no guarantee that we will not break the API again in the future...but I think that this is safe enough and if we break it, we can fix it in another PR...and if someone cares for regression, then we can have a sentry test :)

Only minor comments


Maybe update the docstring for _Code.

- A fake code object, used by L{_Traceback} via L{_Frame}.
+ A fake code object, used by L{_Traceback} via L{_Frame}.
+ It should have the same API as stdlib to allow interaction with
+ other tools.

src/twisted/test/test_failure.py Outdated Show resolved Hide resolved
src/twisted/test/test_failure.py Outdated Show resolved Hide resolved
src/twisted/python/failure.py Show resolved Hide resolved
exarkun and others added 5 commits March 15, 2022 13:59
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
and whitespace cleanup
@exarkun exarkun merged commit f28b36d into twisted:trunk Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants