Closed Bug 1468398 Opened 6 years ago Closed 6 years ago

Support keep alive connections

Categories

(Testing :: geckodriver, enhancement, P3)

Product:
Testing
Component:
geckodriver
Version:
Version 3
Type:
enhancement

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: automatedtester, Assigned: automatedtester)

References

Details

Attachments

(2 files)

Bug 1468398: Allow Keep Alive connections in geckodriver
59 bytes, text/x-review-board-request
jgraham
: review+
Details
Bug 1468398: Enable keep-alive connections in wdclient for wdspec tests.
59 bytes, text/x-review-board-request
whimboo
: review+
ato
: review-
Details
Selenium Java Server has enabled support for keep alive from http 1.1 to help limit port exhaustion especially on Windows. Githug issue https://github.com/mozilla/geckodriver/issues/713
Assignee: nobody → dburns
Priority: -- → P3
Attachment #8985063 - Flags: review?(james)
I think this is still fine to chime into the 0.21.0 release given that people really want it.
IIRC the reason we didn't do this originally is that it breaks some clients, although I 'm having difficulty recalling the details right now. Can you test the patch with the selenium Python testsuite please? I think that was one case that was previously problematic.
jgraham beat me to it. We disabled Keep-Alive connections because of variable support for it in HTTP clients, such as Python’s standard library httplib. httplib is used by the WebDriver client in the WPT tests. I think the Selenium Python client uses something else? The relevant commit disabling Keep-Alive is: https://github.com/mozilla/webdriver-rust/commit/b614a0f45edf60fbc8931688a818edfdc4892cd7
For Wdspec tests which use the WebDriver client it seems to work fine given that all the try jobs are passing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0ec30c5d980b5eac20cb6ed1ff65ed0c4a62ad2 But it would be good to have confidence that we do not regress something for Selenium. So we might not block the geckodriver 0.21.0 release on it.
So if the Java server has added support for HTTP Keep-Alive and Python’s httplib (used by various clients, including in WPT) all support HTTP/1.1 and Keep-Alive, it seems reasonable to re-enable it. automatedtester: Can you test this patch with the Selenium Java- and Python test suites, please?
Flags: needinfo?(dburns)
All those tests pass with keep alive enabled on the client and with keep alive disabled on the client.
Flags: needinfo?(dburns)
Comment on attachment 8985063 [details] Bug 1468398: Allow Keep Alive connections in geckodriver https://reviewboard.mozilla.org/r/250790/#review257182 OK, let's try this, but if we get bug reports about unexpected hangs, this should be the first candidate to revert.
Attachment #8985063 - Flags: review?(james) → review+
Should we land after we released 0.21? I would highly propose so because I see risk in busting the 0.21 release. When we land afterward we have enough time to validate for our in-tree wdspec tests.
Flags: needinfo?(dburns)
If there is a problem, we can just release a patch release with this reverted? It has to go out the door at some point.
I have spoken to :whimboo about this. There is no real risk here as the default clients have it off by default but testing with it on and off doesnt show any regressions. I will see about switching it on by default for wdclient
Flags: needinfo?(dburns)
Comment on attachment 8985414 [details] Bug 1468398: Enable keep-alive connections in wdclient for wdspec tests. https://reviewboard.mozilla.org/r/251046/#review257304 ::: testing/web-platform/tests/tools/webdriver/webdriver/transport.py:148 (Diff revision 1) > payload = body.encode("utf-8") > > if headers is None: > headers = {} > > + headers.update({'Connection': 'keep-alive'}) Bonus point if you can get rid of this extra empty line before.
Attachment #8985414 - Flags: review?(hskupin) → review+
Comment on attachment 8985414 [details] Bug 1468398: Enable keep-alive connections in wdclient for wdspec tests. https://reviewboard.mozilla.org/r/251046/#review257320 ::: testing/web-platform/tests/tools/webdriver/webdriver/transport.py:147 (Diff revision 2) > if isinstance(payload, text_type): > payload = body.encode("utf-8") > > if headers is None: > headers = {} > + headers.update({'Connection': 'keep-alive'}) This only enables Keep-Alive for the lifetime of the current command request. If you want to take advantage of persistent HTTP connections, you need to reuse the HTTPConnection created on :155. Otherwise this patch will make a new connection on for each new request.
Attachment #8985414 - Flags: review-
Pushed by dburns@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3c5863978f1c Allow Keep Alive connections in geckodriver r=jgraham https://hg.mozilla.org/integration/autoland/rev/928ea511d72b Enable keep-alive connections in wdclient for wdspec tests. r=whimboo
David please have a look at the last comment from Andreas. Maybe we should backout the 2nd changeset for now.
Flags: needinfo?(dburns)
So I should add I don’t think the patch as-is is necessarily harmful, but I also don’t think it is working as expected.
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11519 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/3c5863978f1c https://hg.mozilla.org/mozilla-central/rev/928ea511d72b
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
I will do a follow bug to fix. It will still exercise keep-alive though on the client side we wont use it
Flags: needinfo?(dburns)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: