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

errors: change timeout errors from 408 to 500 HTTP status code #1292

Merged
merged 1 commit into from Sep 3, 2018

Conversation

Projects
None yet
2 participants
@andreastt
Member

andreastt commented Aug 21, 2018

Whilst it is logically correct to use 408 for the timeout and script
timeout errors, it causes a collision with HTTP semantics implement
in HTTP clients. To support Keep-Alive we allow retries in HTTP
clients and if a client sees code 408 it thinks that the server has
gone away and retries the request.

This causes Execute Script, Navigate To, and Refresh commands to
be sent twice with some HTTP clients.

This is a backwards incompatible change to WebDriver in order to
not break HTTP/1.1 Keep-Alive.

Fixes: #1287

errors: change timeout errors from 408 to 500 HTTP status code
Whilst it is logically correct to use 408 for the timeout and script
timeout errors, it causes a collision with HTTP semantics implement
in HTTP clients.  To support Keep-Alive we allow retries in HTTP
clients and if a client sees code 408 it thinks that the server has
gone away and retries the request.

This causes Execute Script, Navigate To, and Refresh commands to
be sent twice with some HTTP clients.

This is a backwards incompatible change to WebDriver in order to
not break HTTP/1.1 Keep-Alive.

Fixes: #1287
@andreastt

This comment has been minimized.

Member

andreastt commented Aug 21, 2018

This change is backwards incompatible but we need a fix for this in order to support Keep-Alive and persistent connections. This PR can’t land until we have tests ready and I will supply these downstream in https://bugzilla.mozilla.org/show_bug.cgi?id=1484941.

@andreastt

This comment has been minimized.

Member

andreastt commented Aug 31, 2018

@AutomatedTester The tests have landed.

@AutomatedTester AutomatedTester merged commit c377c21 into w3c:master Sep 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment