1471201 - Increase Keep-Alive connection drop timeout

Status: RESOLVED FIXED

(Testing :: geckodriver, defect, P3)

Description

[Andreas Tolfsen ❲:ato❳]

It was reported in various recent geckodriver issues, including but
not limited to https://github.com/mozilla/geckodriver/issues/1304,
that various (Selenium?) WebDriver clients do not have proper support
for HTTP/1.1 Keep-Alive connections.

Whilst increasing the inactive connection drop timeout duration
will not resolve the above problem (patches are needed in clients),
the default five second timeout that hyper operates with is probably
too low for regular WebDriver use.

whimboo did some investigation of this in
https://github.com/mozilla/geckodriver/issues/1304#issuecomment-400229653
and there is some more background from me in
https://github.com/mozilla/geckodriver/issues/1304#issuecomment-400290438.

Comment 1

[Andreas Tolfsen ❲:ato❳]

The question is, what would a good timeout duration be?

As I understand it the connection is kept alive for as long as it
remains active within the current five second period.  The problem
arises when the client opens a connection, issues a command, then
doesn’t do anything before the timeout elapses.  To slightly rephrase
the question: For how long would we expect a client to not provide
any data?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I actually don't have an idea right now. As already said on IRC if people see that when using eg. `time.sleep()` calls in their Selenium tests, they should first change those to make use of the `Wait` methods.

But something we have to count in for the timeout is clearly the network latency, which in nearly all of the cases shouldn't be that high. 

I would suggest we wait a bit longer and see how enabling keep-alive affects users.

Comment 3

[Andreas Tolfsen ❲:ato❳]

See my comment
https://github.com/mozilla/geckodriver/issues/1304#issuecomment-402685923
about why this change will not fundamentally fix the BadStatusLine
exceptions we are seeing reported in the Selenium Python client.

Due to _the nature_ of how WebDriver works it would benefit users
for the connection to persist longer.  WebDriver tests are usually
rather slow and the lifetime of continuous operations tends to be
longer than your average web document request.  Having to make fewer
HTTP connections for the duration of a test seems like a potential
performance win.

Comment 4

[Andreas Tolfsen ❲:ato❳]

In our meeting on Monday we agreed we would increase the HTTPD
Keep-Alive timeout duration to 60 seconds:
https://wiki.mozilla.org/WebDriver/Meetings/2018-07-02#Minutes

Comment 5

[Andreas Tolfsen ❲:ato❳]

Apparently the default Keep-Alive timeout in a more recent version
of hyper is 90 seconds.  I propose we use that number so that when
we upgrade to this newer version in the future, we will get the
hyper default without having to change this value twice.

Comment 6

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1471201 - Increase webdriver HTTPD Keep-Alive timeout to 90s.

Increasing geckodriver's Keep-Alive timeout duration to 90
seconds, from the default five seconds, will reduce the number of
HTTP connections a persistent-enabled client will have to make,
potentially boosting performance.

In more recent hyper versions the default is 90 seconds, which
means we can get rid of this line when hyper is upgraded.

This will help mitigate https://github.com/mozilla/geckodriver/issues/1304
but not fundamentally resolve it, due to a standard library bug in
Python 2.7's urllib.

Review commit: https://reviewboard.mozilla.org/r/254974/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254974/

Comment 7

[David Burns :automatedtester]

Comment on attachment 8989966 [details]
Bug 1471201 - Increase webdriver HTTPD Keep-Alive timeout to 90s.

https://reviewboard.mozilla.org/r/254974/#review261832

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6637d206368f
Increase webdriver HTTPD Keep-Alive timeout to 90s. r=automatedtester

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/6637d206368f