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

Print stacktrace when catching TimeoutError #3414

Merged
merged 4 commits into from
Dec 1, 2018

Conversation

bitrut
Copy link
Contributor

@bitrut bitrut commented Nov 29, 2018

What do these changes do?

When TimeoutError is caught in the handler, the exception stack trace is logged.

Are there changes in behavior for the user?

Related issue number

No related issue.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@bitrut bitrut force-pushed the timeout_error_stacktrace branch from c5e193b to 6fc0333 Compare November 29, 2018 19:40
@codecov-io
Copy link

codecov-io commented Nov 29, 2018

Codecov Report

Merging #3414 into master will decrease coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3414      +/-   ##
==========================================
- Coverage   98.01%   97.89%   -0.12%     
==========================================
  Files          44       44              
  Lines        8511     8511              
  Branches     1382     1382              
==========================================
- Hits         8342     8332      -10     
- Misses         70       76       +6     
- Partials       99      103       +4
Impacted Files Coverage Δ
aiohttp/web_protocol.py 92.2% <100%> (ø) ⬆️
aiohttp/tcp_helpers.py 90% <0%> (-6.67%) ⬇️
aiohttp/web_fileresponse.py 96.51% <0%> (-1.75%) ⬇️
aiohttp/client_reqrep.py 96.96% <0%> (-0.49%) ⬇️
aiohttp/connector.py 97.25% <0%> (-0.35%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f09cec5...933dbda. Read the comment docs.

@asvetlov
Copy link
Member

Timeout is a pretty common situation.
Logging a stack trace every time can be annoying.
Maybe keeping a trace in debug mode only makes sense?

Also a test is required, at least to never miss the functionality on future refactorings.

@bitrut
Copy link
Contributor Author

bitrut commented Nov 29, 2018

@asvetlov
My fix is the result of very long investigation of occasional tests failures in my project, which happened only on CI.
Step by step I got the point that I have set too short timeouts for the db connection.
But seeing the handler responding with http 504 without any context wasn't too helpful.

I'll try to follow your hints.

@bitrut
Copy link
Contributor Author

bitrut commented Nov 29, 2018

@asvetlov
I added suggested changes.

@bitrut bitrut force-pushed the timeout_error_stacktrace branch from f5634b1 to 933dbda Compare November 30, 2018 00:19
@asvetlov asvetlov merged commit 66d1f9c into aio-libs:master Dec 1, 2018
@asvetlov
Copy link
Member

asvetlov commented Dec 1, 2018

Thanks!

@lock
Copy link

lock bot commented Dec 1, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Dec 1, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 1, 2019
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Dec 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bot:chronographer:provided There is a change note present in this PR outdated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants