Raise cancelled on closed

[azhpushkin]

What do these changes do?

Add raising of asyncio.CancelledError to PayloadWriter write() method to avoid writing to closing transport.

Are there changes in behavior for the user?

According to #2499 (comment) is was possible to catch this situation earlier by manually checking transport status of request. Thus, change should not broke any previously written code.

Related issue number

Resolves #2499

Checklist

• [*] I think the code is well written
• [*] Unit tests for the changes exist
• [*] Add a new news fragment into the CHANGES folder

[codecov-io]

Codecov Report

Merging #2640 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2640      +/-   ##
==========================================
+ Coverage   97.92%   97.92%   +<.01%     
==========================================
  Files          38       38              
  Lines        7265     7268       +3     
  Branches     1261     1262       +1     
==========================================
+ Hits         7114     7117       +3     
  Misses         47       47              
  Partials      104      104

Impacted Files	Coverage Δ
aiohttp/http_writer.py	99.24% <100%> (+0.01%)	⬆️

---

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 b78228f...e4f3245. Read the comment docs.

[pfreixes]

LGTM changes approved, thanks!

Branch out of date

[pfreixes]

but please first I would ask you to synchronize your branch with the master one.

[azhpushkin]

@pfreixes Done! (i guess merge is enough for further rebase&squash? )

[pfreixes]

thanks!

[asvetlov]

Hold on!
Is CancelledError raised by client API?
CancelledError without actual cancellation on resp.write() is confusing also.
I suggested raising RuntimeError or maybe even better a custom exception type derived from RuntimeError.

[azhpushkin]

@asvetlov
Agree, make sence. But why RuntimeError and not ConnectionError, for example? It could be a good match.

[pfreixes]

Taking into account that write_eof calls the drain and it can have as a side effect a cancellation error if the client disconnects, don't we have to try to handle both cases in such a way that the developer can catch easily them using just one exception?

[pfreixes]

Yeps forgot my last comments, drain as other waiters are wake up with the proper exception once the connection gets closed. Should we use a ConnectionError [1] exception or one of its children?

[1] https://docs.python.org/3/library/exceptions.html#ConnectionError

[azhpushkin]

asyncio's FlowControlMixin uses ConnectionResetError, so we could use it too, but I have a concern whether transport could be closed only on the other than aiohttp side (otherwise usual ConnectionError looks as a better match to me).

[davepallot]

How do you distinguish between a local task cancel and a remote socket disconnect? I have an app which needs to make that distinction.

[asvetlov]

No way right now.
But you didn't cancel a task -- something canceled it from outside :)

[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