Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for HTTP/1.1 and random non-responsive devices #197 #216

Merged
merged 1 commit into from
Jan 25, 2020

Conversation

bdraco
Copy link
Contributor

@bdraco bdraco commented Dec 4, 2019

Homekit needs the headers and content sent
in a single write call or it will randomly
get connection reset by peer which leads
to iOS stalling and devices showing up
as non-responsive.

Without HTTP/1.1 keep-alives, 50+ devices
would cause a bridge to timeout or devices
to go non-responsive because homekit would
have to make a new connection to get / put
for each device. With HTTP/1.1 it can avoid
all of this overhead

After this change, devices in homekit on
the bridges with > 80 devices were
now immediately responsive in testing.

@codecov-io
Copy link

codecov-io commented Dec 4, 2019

Codecov Report

Merging #216 into dev will increase coverage by 0.65%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##              dev     #216      +/-   ##
==========================================
+ Coverage   60.97%   61.63%   +0.65%     
==========================================
  Files          16       16              
  Lines        1694     1697       +3     
  Branches      175      175              
==========================================
+ Hits         1033     1046      +13     
+ Misses        622      612      -10     
  Partials       39       39
Impacted Files Coverage Δ
pyhap/hap_server.py 36.9% <100%> (+2.46%) ⬆️

Homekit needs the headers and content sent
in a single write call or it will randomly
get connection reset by peer which leads
to iOS stalling and devices showing up
as non-responsive.

Without HTTP/1.1 keep-alives, 50+ devices
would cause a bridge to timeout or devices
to go non-responsive because homekit would
have to make a new connection to get / put
for each device.  With HTTP/1.1 it can avoid
all of this overhead

After this change, devices in homekit on
the bridges with > 80 devices were
now immediately responsive in testing.
Comment on lines +204 to +218
# Important: we need to send the headers and the
# content in a single write to avoid homekit
# on the client side stalling and making
# devices appear non-responsive.
#
# The below code does what end_headers does internally
# except it combines the headers and the content
# into a single write instead of two calls to
# self.wfile.write
#
# TODO: Is there a better way that doesn't involve
# touching _headers_buffer ?
#
self.connection.sendall(b"".join(self._headers_buffer) + b"\r\n" + bytesdata)
self._headers_buffer = []
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really needed? I.e. how often do you see a stall if this is not done?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would see it drop the connection and then end up in a stall state for a while until iOS hit an internal logic to reconnect. It would happen about once a week and I'd usually have to reboot the phone to make everything work again. This fix is similar to an issue I've seen before with apple's webdav library.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The underlying issue is likely a bug in apple's iOS libraries that isn't dealing well with data coming at unexpected times (reading when its expecting to write and vise-versa). It is also likely the reason #215 also causes disconnects. Apple developer support says its a "known" issue that is being worked on.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey thanks for the insights, sounds good!

@ikalchev
Copy link
Owner

One more thing - which python did you test with?
PS: Sorry for the delay on this, I hope we can merge and release this by end of the week.

Copy link
Contributor Author

@bdraco bdraco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with two versions of python

home-assistant# python3 --version
Python 3.7.6

dev1# python3 --version
Python 3.6.8

@ikalchev ikalchev merged commit 2d880ae into ikalchev:dev Jan 25, 2020
@ikalchev
Copy link
Owner

Thank you for the change, I will release the new version in pip over the weekend.

@ikalchev ikalchev mentioned this pull request Apr 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants