aiohomekit icon indicating copy to clipboard operation
aiohomekit copied to clipboard

Use an existing library for HTTP/1.1 header parsing

Open ntninja opened this issue 3 years ago • 4 comments

One of the issues fixed in #9 could have easily be prevented by not employing a roll-your-own HTTP header parsing library. While the custom framing requirements making using a full-blown HTTP client undesirable, at the HTTP parsing/generation could be delegated to a common library maintained by “somebody else” and h11 seems like the perfect tool for this job as it (I know at least one “serious” HTTP library that uses it).

ntninja avatar Jul 30 '20 23:07 ntninja

Interesting. Will it handle a stream of interspersed EVENT/1.0 and HTTP/1.1 messages? Regarding the generation part it really depends how much control it gives, will have a look soon. We have to talk to a lot of very broken hap implementations, and often that means trying to be byte for byte compatible with what iOS does. Aren't there yet with that but it's looking more and more inevitable.

Not applicable to HTTP handling but e.g. recently had a case where a thermostat couldn't handle a space in a valid JSON body. (Actually only some API endpoints were affected by this). A different thermostat would accept 1, 0 and false for a boolean field. But not true. The spec says all 4 are valid, but we now have to send 1 or 0 only which is what the implementation does. We've had some issues with the Host header in the past too.

So try to be as close to the iOS implementation as possible for sending and as forgiving as possible with reading.

There is a lot of stuff like it is because this is a fork of homekit_python. The plan was to do quite a bit of refactoring but my time has been swallowed up so I'm not really actively improving it as I had hoped just reacting to bugs. So code improvements very much welcome.

Jc2k avatar Jul 30 '20 23:07 Jc2k

As suspected, it looks h11 would not work for this particular HTTP dialect (directly). E.g.

https://github.com/python-hyper/h11/blob/master/h11/_abnf.py#L84

This is used in a regex here (via status_line, defined a few lines below http_version):

https://github.com/python-hyper/h11/blob/41a3467dd98271529cce361c756da9e34d8e2b00/h11/_readers.py#L89

As you probably know, when a HomeKit accessory notifies a controller about an event it violates the HTTP spec and sends a response without a request. This is one of the other reasons we don't use an existing client - they don't tend to like that as they expect strict request/response. As you say it would be nice to use the parsing. But it's not a HTTP/1.1 message, its an EVENT/1.0 message. In every other way, it is HTTP/1/1 compatible, apart from most parsers don't expect the 5 bytes to be anything other than HTTP/.

You could monkey patch, but that's a terrible idea, especially as the main aim of aiohomekit is to work well in Home Assistant. It may well use h11 itself indirectly.

We could peek at hte data before passing it to h11. That also seems quite grim.

Jc2k avatar Aug 03 '20 08:08 Jc2k

In general the dirty horrible nasty worse thing in the world HTTP parser seems to be coping with real world HAP devices. It has struggled most with homebrew non-certified HAP servers, but the changes needed to support them have been minor.

At the same time vendors attempts at HAP have been quite bad at working with anything that isn't an iOS device, even when we are blatantly following the spec. As we don't have a large test pool, changes are scary.

So at the moment i'm reluctant to change this, especially if it requires doing something awkward to use h11 for event notifications.

I must admit i haven't tried h11, maybe there is some API that avoids the regex i highlighted. I'll leave this open with a help wanted tag for now.

Jc2k avatar Aug 03 '20 08:08 Jc2k

I tried h11 as we use it in HAP-python. I gave up on this.

The device in https://github.com/home-assistant/core/issues/46785 seems like its tripping up the parser.

I'm going to try to get my hands on one of those

bdraco avatar May 24 '21 21:05 bdraco