Skip to content

Conversation

@Dreamsorcerer
Copy link
Member

@Dreamsorcerer Dreamsorcerer commented Jan 21, 2024

Should help improve some bug reports in future.

@Dreamsorcerer Dreamsorcerer requested a review from bdraco January 21, 2024 19:59
@Dreamsorcerer Dreamsorcerer added the bot:chronographer:skip This PR does not need to include a change note label Jan 21, 2024
@codecov
Copy link

codecov bot commented Jan 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f530e92) 97.42% compared to head (a5307db) 97.48%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8049      +/-   ##
==========================================
+ Coverage   97.42%   97.48%   +0.06%     
==========================================
  Files         107      107              
  Lines       32598    32600       +2     
  Branches     3800     3800              
==========================================
+ Hits        31757    31781      +24     
+ Misses        634      616      -18     
+ Partials      207      203       -4     
Flag Coverage Δ
CI-GHA 97.40% <100.00%> (+0.05%) ⬆️
OS-Linux 97.07% <100.00%> (+0.05%) ⬆️
OS-Windows 95.57% <100.00%> (+<0.01%) ⬆️
OS-macOS 96.89% <100.00%> (+0.07%) ⬆️
Py-3.10.11 95.50% <100.00%> (+<0.01%) ⬆️
Py-3.10.13 96.87% <100.00%> (+<0.01%) ⬆️
Py-3.11.7 96.56% <100.00%> (+0.02%) ⬆️
Py-3.12.1 96.69% <100.00%> (+<0.01%) ⬆️
Py-3.8.10 95.47% <100.00%> (?)
Py-3.8.18 96.81% <100.00%> (+<0.01%) ⬆️
Py-3.9.13 95.47% <100.00%> (+<0.01%) ⬆️
Py-3.9.18 96.85% <100.00%> (+0.03%) ⬆️
Py-pypy7.3.15 96.40% <100.00%> (?)
VM-macos 96.89% <100.00%> (+0.07%) ⬆️
VM-ubuntu 97.07% <100.00%> (+0.05%) ⬆️
VM-windows 95.57% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Dreamsorcerer
Copy link
Member Author

We could really do with a way to also get the traceback added in StreamReader.set_exception(). Can't seem to figure it out at the moment, but ideally we'd grab the stack trace in that function and then add it the exc.__cause__ somehow.

@Dreamsorcerer
Copy link
Member Author

Maybe at worst we can just add:

try:
    raise Exception()
except Exception as e:
    exc.__cause__ = e

Though feels like quite an awkward way to achieve it.

@bdraco
Copy link
Member

bdraco commented Jan 21, 2024

Maybe at worst we can just add:

try:
    raise Exception()
except Exception as e:
    exc.__cause__ = e

Though feels like quite an awkward way to achieve it.

That seems awkward for sure. traceback.extract_tb might work but its also really expensive.. sys._getframe is cheap but only works on cpython.

Setting __cause__ is cheap and works everywhere.. its just a lot of copy pasta.

Copy link
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

This looks good

@Dreamsorcerer Dreamsorcerer merged commit a379e63 into master Jan 21, 2024
@Dreamsorcerer Dreamsorcerer deleted the exception-cause branch January 21, 2024 21:05
@patchback
Copy link
Contributor

patchback bot commented Jan 21, 2024

Backport to 3.9: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.9/a379e6344432d5c033f78c2733fe69659e3cff50/pr-8049

Backported as #8050

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Jan 21, 2024
@patchback
Copy link
Contributor

patchback bot commented Jan 21, 2024

Backport to 3.10: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.10/a379e6344432d5c033f78c2733fe69659e3cff50/pr-8049

Backported as #8051

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Jan 21, 2024
Dreamsorcerer pushed a commit that referenced this pull request Jan 21, 2024
Dreamsorcerer pushed a commit that referenced this pull request Jan 21, 2024
@webknjaz
Copy link
Member

I think, this one may need a change note.
Also, I seem to recall finding other places where it's unset, in that old issue. Have you looked into it?

@Dreamsorcerer
Copy link
Member Author

Not that I've seen, I just noticed a lack of information while debugging a couple of issues. The set_exception()s are the main other ones, but as described above it's a bit awkward to figure out how to put the traceback in. Best left to a separate PR if we do it.

@webknjaz
Copy link
Member

@Dreamsorcerer I was thinking about all the other places as mentioned @ #4581 (comment). There's a set_exception() helper that isn't always used, it seems, and could be extended to be slightly more generic per my comment there.

Also, why didn't you reuse my patch @ #4581 (comment)? It's almost the same, but with better var names and the original exception repr backed into the message.

@Dreamsorcerer
Copy link
Member Author

I was thinking about all the other places as mentioned @ #4581 (comment).

Oh, I totally forgot about that conversation. I was just debugging something on my own yesterday and found that one missing. Doesn't look like you solved the .set_exception() yet though.

@webknjaz
Copy link
Member

Doesn't look like you solved the .set_exception() yet though.

@Dreamsorcerer I ended up pushing #8089. I haven't tried it, though. And didn't add any tests. Plz check it for obvious mistakes.

Setting __cause__ is cheap and works everywhere.. its just a lot of copy pasta.

@bdraco my PR unifies that a bit. Interestingly, I've found the use of __context__ in one place that I suspect may be problematic...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:skip This PR does not need to include a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants