Critic review:https://critic.hoppipolla.co.uk/r/498

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please donot make in-place history rewrites (e.g. viagit rebase -i orgit commit --amend) when updating this pull request.

Urgh, looking at this again I must've been thinking of my somewhat-working-written-a-long-time-ago Python 3-only rewrite of the input stream. Dealing withfirstChunk is going to be hideously complex.

Alternative idea for fixing this (comments welcome, don't take this as me saying what the right approach is — I don't know what the right approach is!): make BufferedStream generic and work for both unicode and bytes file-like objects (literally the only thing specific is theb"".join at the end!), make sure it hides bugs whereread is broken (so it guaranteeslen(s.read(n)) <= n), and then wrapall non-seekable streams with it inHTMLInputStream, which then meansread(0) is safe for all non-seekable streams.

The other option is to try and finish my long-abandoned Python 3-only rewrite of inputstream, create something with the same API (but slower when changing character encoding) for Python 2.https://github.com/gsnedders/html5lib-python/tree/new-inputstream is what there is. Hopefully shows the vague idea.

At the momentBufferedStream is only used when the input stream is non-seekable, so it doesn’t cover all cases.

Hmm, looking ataddinfourl, I wonder if we should just detect the bug withif isinstance(source, http_client.HTTPResponse) or source.read == http_client.HTTPResponse. This would handle both any cases of people subclassingHTTPResponse as well as handlingaddinfourl. Do we actually want to work around more general cases of people breaking the file-like object API contract? I'd argue not… This is only really special in that it is stdlib code breaking it.

At one point I hadif isinstance(six.get_method_self(source.read), http_client.HTTPResponse), but that broke on objects wheresource.read was not a method.

Is there any reason not to just go for the equality check?

Do you meanif source.read == http_client.HTTPResponse.read? That doesn’t work,source.read is a bound method.

So, what,get_method_self would raiseAttributeError, no? Can we not just check for that?

We can, it’s just one more corner case piling up…

How about this?

# Work around Python bug #20007: read(0) closes the connection.if (isinstance(source, http_client.HTTPResponse) or     # Also check for addinfourl wrapping HTTPResponse    isinstance(getattr(source, 'fp', None), http_client.HTTPResponse)):

It should never raise, and cover every case where we’ve seen 20007 so far.

I guess it is as good as anything. As ugly as it is.

I'm so confused as to what's happened here. Why did this get dropped? Was I expecting you to update PR with that? I don't know. Looking over this again,get_method_self seems sanest, along with moving it within the followingif. Closing this to replace it with#186.

Oh, no, I'm being stupid. I'm thinking about JS semantics and not Python ones!