headers_received signal

[elacuesta]

Closes #1772

A new headers_received signal that fires when the headers for a response are received, before starting the download of the body. Handlers for this signal can stop the download of a response by raising a scrapy.exceptions.StopDownload exception, just like the handlers for the bytes_received signal.

Tasks:

• Implementation
• Tests
• Docs
• Fix failing tests

[Gallaecio]

Nice.

(not merging yet due to on-going tests)

[codecov]

Codecov Report

Merging #4897 (d60b697) into master (ea92b49) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4897      +/-   ##
==========================================
+ Coverage   87.82%   87.83%   +0.01%     
==========================================
  Files         158      158              
  Lines        9723     9736      +13     
  Branches     1433     1435       +2     
==========================================
+ Hits         8539     8552      +13     
  Misses        929      929              
  Partials      255      255              

Impacted Files	Coverage Δ
scrapy/core/downloader/handlers/http11.py	92.69% <100.00%> (+0.22%)	⬆️
scrapy/signals.py	100.00% <100.00%> (ø)
scrapy/utils/curl.py	100.00% <100.00%> (ø)

[Gallaecio]

So glad you managed to find the issue affecting the tests! 🎉 I had made some attempts myself but I had not been able to.

[elacuesta]

@Gallaecio please review again after 84e91b6 😄

[Gallaecio]

I’m undecided on the right approach to provide the content length.

My initial reaction was: we should first fix Scrapy responses missing the Content-Length header. Then I saw https://github.com/Almad/twisted/blob/d9dccb23ffa63184e376e98bebe6a5cee32953f3/twisted/web2/channel/http.py#L202 and I thought it made sense, Twisted is just following the standard. Then I thought of cases where the header and the content length do not actually match, and how in Scrapy we allow our users to still process those requests; I think exposing the Content-Length header to users would go in line with that, Scrapy users some times may need that low-level stuff.

So I’m +0.5 on this. I think the code looks great, I’m just not sure about the length parameter. Adding it as a parameter would require a messy deprecation later if we eventually remove it. But the only alternative I can think of would be to add the header to responses, which I think would be out of the scope of this change, and I’m unsure whether it would be the easiest thing (pick the content length like you did here and set that into the header) or require Twisted monkey patching. If it’s not easy, it’s probably best to have a messy deprecation later than to postpone this further.