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

Signals refacoting #2480

Merged
merged 5 commits into from Nov 9, 2017
Merged

Signals refacoting #2480

merged 5 commits into from Nov 9, 2017

Conversation

@asvetlov
Copy link
Member

@asvetlov asvetlov commented Nov 8, 2017

  • Convert more API to async/await syntax
  • Simplify and speedup signals implementation
  • Drop support for non-coroutine signal handlers

Fix for #2419

@asvetlov
Copy link
Member Author

@asvetlov asvetlov commented Nov 8, 2017

@pfreixes the RP breaks your work on client tracing but I pretty sure the change is not big and is done for good.

Copy link
Member

@kxepal kxepal left a comment

Should changes mention drop on_pre_signal and on_post_signal?

@asvetlov
Copy link
Member Author

@asvetlov asvetlov commented Nov 8, 2017

Sure.
These signals was never documented and I pretty sure nobody uses it.

@codecov-io
Copy link

@codecov-io codecov-io commented Nov 8, 2017

Codecov Report

Merging #2480 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2480      +/-   ##
==========================================
+ Coverage   97.14%   97.15%   +0.01%     
==========================================
  Files          39       39              
  Lines        8136     8097      -39     
  Branches     1420     1416       -4     
==========================================
- Hits         7904     7867      -37     
+ Misses        101       99       -2     
  Partials      131      131
Impacted Files Coverage Δ
aiohttp/test_utils.py 98.57% <100%> (ø) ⬆️
aiohttp/worker.py 96.12% <100%> (+0.03%) ⬆️
aiohttp/signals.py 100% <100%> (+4.25%) ⬆️
aiohttp/web.py 98.96% <100%> (-0.03%) ⬇️

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 78d08ce...a9b3694. Read the comment docs.

class PostSignal(DebugSignal):

__slots__ = ()
await receiver(*args, **kwargs)

This comment has been minimized.

@kxepal

kxepal Nov 8, 2017
Member

How about to add more friendly exception if plain func is going to be registered as handler? I guess now such code will fail with cryptic TypeError: object NoneType can't be used in 'await' expression or similar.

This comment has been minimized.

@asvetlov

asvetlov Nov 8, 2017
Author Member

Do you suggest catching TypeError and raise a new TypeError with more informative exception message?

This comment has been minimized.

@asvetlov

asvetlov Nov 8, 2017
Author Member

As an option we could check ll callbacks on freezing stage.
It prevents registering a regular function which returns a future but it's fine I guess.

This comment has been minimized.

@kxepal

kxepal Nov 8, 2017
Member

Yes for TypeError and yes, better check this early. On append call if possible. The stacktrace will help to find that bad signal usage with easy.

This comment has been minimized.

@asvetlov

asvetlov Nov 8, 2017
Author Member

FrozenList can be modified by too many ways, .append is not the only one.
I think .freeze() is a good compromise.

This comment has been minimized.

@kxepal

kxepal Nov 8, 2017
Member

It's a pity. Ok then if error message would contains function name which is not async.

This comment has been minimized.

@asvetlov

asvetlov Nov 8, 2017
Author Member

Well, but I see you point.
Maybe we need specialized CheckedFrozenList class with registered callback? It could improve UX slightly.
Please let me merge the PR as is and make a new issue.
I don't want to block @pfreixes with his client tracing PR.

This comment has been minimized.

@kxepal

kxepal Nov 8, 2017
Member

Ok. Let's do that.

@kxepal
kxepal approved these changes Nov 8, 2017
@pfreixes
Copy link
Contributor

@pfreixes pfreixes commented Nov 8, 2017

LGTM. Just take into account that Func signals might be needed later, but let's implement it once we know for sure that are gonna needed.

@asvetlov asvetlov merged commit 5c21369 into master Nov 9, 2017
5 checks passed
5 checks passed
codecov/patch 100% of diff hit (target 97.14%)
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@asvetlov asvetlov deleted the signals-refactoring branch Nov 9, 2017
@socketpair
Copy link
Contributor

@socketpair socketpair commented May 16, 2018

@asvetlov what was intention to remove support of synchronous functions? We had to refactor all our backends due to this decision. Why not to write await coroutine(receiver(*args, **kwargs)) ?

Also, about automatic detection: lambda app: app['xxx'].stop() in our case actually returns coroutine, since stop() is a coroutine. But it can not be detected until someone calls that lambda.

@asvetlov
Copy link
Member Author

@asvetlov asvetlov commented May 16, 2018

  1. API simplicity
  2. A check for coroutine is quite expensive
@lock
Copy link

@lock lock bot commented Oct 28, 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].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants