Signals refacoting

[asvetlov]

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

Fix for #2419

[asvetlov]

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

[kxepal]

Should changes mention drop on_pre_signal and on_post_signal?

[asvetlov]

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

[codecov-io]

Codecov Report

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

@@            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.

[kxepal]

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.

[asvetlov]

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

[asvetlov]

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.

[kxepal]

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.

[asvetlov]

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

[kxepal]

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

[asvetlov]

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.

[kxepal]

Ok. Let's do that.

[pfreixes]

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.

[socketpair]

@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]

1. API simplicity
2. A check for coroutine is quite expensive

[lock]

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