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

Set cause for ClientPayloadError #8049

Merged
merged 1 commit into from
Jan 21, 2024
Merged

Set cause for ClientPayloadError #8049

merged 1 commit into from
Jan 21, 2024

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
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
32 of 34 checks passed
@Dreamsorcerer Dreamsorcerer deleted the exception-cause branch January 21, 2024 21:05
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
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.

3 participants