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

HTTP status code 408 causes clients to retry #1287

Closed
barancev opened this issue Aug 15, 2018 · 8 comments
Closed

HTTP status code 408 causes clients to retry #1287

barancev opened this issue Aug 15, 2018 · 8 comments
Labels
bug

Comments

@barancev
Copy link

@barancev barancev commented Aug 15, 2018

Section 6.6 contains two error codes "script timeout"and "timeout" mapped to HTTP status code 408 Request Timeout.

It might be logically correct to use this error code, but we have a collision with HTTP semantics implemented 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 and attempts to retry the request. As a results, we have executeScript or get/refresh duplication after timeout.

What can we do?

  1. Stop using keep-alive, and hit the famous port exhaustion problem.
  2. Stop using code 408 and replace it with a more neutral one that does not cause retries.
@andreastt
Copy link
Member

@andreastt andreastt commented Aug 16, 2018

Which HTTP code do you propose?

@andreastt andreastt added the bug label Aug 16, 2018
@barancev
Copy link
Author

@barancev barancev commented Aug 16, 2018

Unfortunately there is no HTTP code that would perfectly fit the purpose. We can use 400, as we do for the most other errors.

@barancev
Copy link
Author

@barancev barancev commented Aug 16, 2018

Or 500, because timeout is a server error, not a client one.

@andreastt
Copy link
Member

@andreastt andreastt commented Aug 16, 2018

I would prefer 500 over 400, since the latter gives the impression of a problem with the client’s request (e.g. malformed data or invalid arguments).

Can we get some input from @kereliuk, @burg, and @jimevans?

@andreastt
Copy link
Member

@andreastt andreastt commented Aug 16, 2018

Sorry, forgot @InstyleVII.

@burg
Copy link
Contributor

@burg burg commented Aug 20, 2018

I would prefer 500 over 400, per Andreas' argument.

@jimevans
Copy link
Contributor

@jimevans jimevans commented Aug 20, 2018

I don't know that I have a preference, but @andreastt makes a good argument. I'm perfectly happy using 500.

andreastt added a commit to andreastt/webdriver that referenced this issue 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: w3c#1287
barancev added a commit to SeleniumHQ/selenium that referenced this issue Aug 22, 2018
AutomatedTester added a commit that referenced this issue Sep 3, 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
@andreastt
Copy link
Member

@andreastt andreastt commented Sep 3, 2018

For the interested, I also wrote a blog post about this change: https://sny.no/2018/08/webdriver-keepalive

grigaman pushed a commit to grigaman/selenium that referenced this issue Sep 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants