Dont truncate received http body to match CL header value if in inspecting mode

[izuzak]

This enables thor to pass the whole body to redbot in case the value of the CL header is smaller than len(body). Currently, redbot thinks that the CL header value matches the length of the body (since only a prefix of the body is passed to it), which is not true.

[mnot]

I don't think this is going to work as-is; it effectively makes persistent connections unsafe in inspecting mode.

I think the only way to really detect this reliably is to run Thor with a connection-per-request model -- i.e., use Connection: close and compare Content-Length to the number of bytes on the wire.

That would be a pretty big performance hit for redbot, so I'm not sure we want to do it by default (although it's worth talking about...)

One way to mitigate that might be to keep it using persistent connections as-is, but when there's an error parsing the subsequent message, to re-issue the request with connection: close to do the comparison.

[izuzak]

Oh, you're definitely right. Persistent connections haven't even crossed my mind, obviously.

The connection-per-request solution is what first came to my mind, also. As for the performance implications - I'm thinking that inspecting mode kind-of implies a non-focus on speed (rather on completeness and fault tolerance). It's hard for me to tell where the line is.

The retry-on-error approach sounds like a more complex change and introducing this kind of complexity just for supporting one use case might not be worth it. Plus - I'm not sure that it would work for all cases. The solution assumes that there will be a subsequent message on the same connection, which might not be the case, right? Also, the problem is that if thor uses the subsequent message to "detect" an error, it has already reported that the previous (i.e. current) response was done and has truncated its body, leading redbot to think that everything was ok (when in fact it wasn't). So, basically, thor would have to hold off reporting the status of the current response until the subsequent response is done?

[izuzak]

I'm closing this pull request as it is obviously not the expected way to fix this issue. And I'm not sure that I understand the codebase well enough to make a different suggestion (see previous message).

[mnot]

Sorry for the delay; been holidaying and working.

It's not an easy decision, you're right.

I've been thinking about having a 'deep' mode to do more specific tests; this could be included in that.

Also, it might be worth moving to a request-per-connection model on the single-resource test, since it doesn't make that many requests...