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

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

Merged
merged 1 commit into from Sep 3, 2018
Merged

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

merged 1 commit into from Sep 3, 2018

Conversation

andreastt
Copy link
Member

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

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
Copy link
Member Author

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
Copy link
Member Author

@AutomatedTester The tests have landed.

@AutomatedTester AutomatedTester merged commit c377c21 into w3c:master Sep 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants