Force HTTP/1.1

[abaker]

Quite frequently when I am on mobile data I will experience #4669. The incremental sync state will be idle, but the progress bar will be visible for one minute before throwing a timeout exception and displaying a connectivity error bar

I found some open OkHttp issues that were possibly related (3146, 3278 and 4455). I didn't drop links because I don't know if they're the cause, but these led me to disabling HTTP/2 which seems to resolve the issue.

I have attached a recording with the following setup:

• Phone1 as a mobile hotspot
• Phone2 in airplane mode connected to hotspot, proxying connections through a laptop to record traffic

I'm running two builds of element in split screen on phone2. The top app (dark mode) is this PR, the bottom app (light mode) is v1.3.14 from the Play Store. The apps show a blue airplane mode bar instead of a red connectivity error bar because I'm connecting through the hotspot.

Based on the proxy traffic it appears that phone1 briefly loses internet connectivity and the HTTP/2 sync request gets stuck for some reason.

It may be possible to fix this without disabling HTTP/2, e.g. listening for connectivity changes and evicting the OkHttp connection pool.

element-4669-repro.mp4

---

• Changes has been tested on an Android device or Android emulator with API 21
• UI change has been tested on both light and dark themes
• Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
• Pull request is based on the develop branch
• Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
• Pull request includes screenshots or videos if containing UI changes
• Pull request includes a sign off
• You've made a self review of your PR
• If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

[bmarty]

@T-bond you down voted the PR what is your concern?

[T-bond]

@bmarty ups. Sorry, I accidentally misclicked the reaction, and did not notice. I don't think we will loose anything forcing HTTP 1.1

[BillCarsonFr]

FTR probably related to square/okhttp#3278 and square/okhttp#4455 and square/okhttp#3146

[bmarty]

FTR some internal feedback:

based on my understanding of how okhttp manages connections, it's probably fine to use HTTP/1.1 instead of HTTP/2, in that I don't expect this to result in masses of TCP connections being created as I suspect okhttp does make use of persistent connections with existing connections in the connection pool. If I were you I would mock up a basic test jig to confirm this though: with a very simple HTTP server which holds open connections for 30s or so then just make a bunch of requests to it using your okhttp configuration and make sure you only see a max of N TCP connections in netstat or equiv, where N = the connection pool limit

longer term, I would be concerned that okhttp has issues like this open for 3+ years. Your choice of HTTP library is utterly critical to the functioning of the app, so if there's big failure modes like "you can't send HTTP requests for N minutes whilst things timeout" then this is probably an indication that maybe you should be looking for an alternative

https://github.com/google/volley looks to be google maintained

[abaker]

There is some documentation for okhttp connections here.

Charles proxy lists a client connection number and server connection number for each request, and I can see multiple http/1.1 calls with matching connection numbers. I assume this means connections are being reused.

[bmarty]

Thanks for your test @abaker 👍