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

[🚀 Feature]: Configurable HTTP Client settings across bindings #12368

Open
titusfortner opened this issue Jul 14, 2023 · 19 comments
Open

[🚀 Feature]: Configurable HTTP Client settings across bindings #12368

titusfortner opened this issue Jul 14, 2023 · 19 comments

Comments

@titusfortner
Copy link
Member

titusfortner commented Jul 14, 2023

Status

Python: #13286 (partial)
Java:
Ruby:
JS:
.NET:

Feature and motivation

Update: Here's the example page describing client settings and what all examples need to be added and what features still need to be implemented: WIP - HTTP Client Documentation

I went down a rabbit hole of timeout settings in Selenium between the different bindings, and what the defaults are and what can be configured. Edit: added WebDriverIO for reference

Language  Max Redirects Read Timeout Configurable
Python 3 No
Ruby 20 60 Timeout, not redirects
Java 100 180 Timeout, not Redirects
.NET 50 60 No
JS No
(WDIO) 3 120 Yes
recommended 20 120 Yes!

According to Jari, Max Redirect should be 20.(93eee69)

It is also problematic that the default page load timeout is 300 when the read timeout in most languages is so much less (the driver will wait for the page long after the code errors and can't send a quit command). I'm not sure why it was set that high in the first place? I think we should change the defaults across the board to be 120 second read timeout and 115 second page load timeout.

Usage example

ClientConfig config = ClientConfig.defaultConfig().readTimeout(Duration.ofSeconds(300)).redirects(25).baseUrl(url)
http_client = Selenium::WebDriver::Remote::Http::Default.new(timeout: 300, redirects: 25)
client_config = ClientConfig()
client_config.timeout = 300
client_config.redirects = 25
@titusfortner
Copy link
Member Author

@christian-bromann does JS itself have defaults here, or did you completely add these?

@christian-bromann
Copy link
Contributor

does JS itself have defaults here, or did you completely add these?

We are using a request library which I believe has some defaults in that regards. These particular timeouts are set by me. If you have better suggestions for them, let me know.

@titusfortner
Copy link
Member Author

The tests I did, I couldn't get JS to time out, and I gave up after a bit. @AutomatedTester do you and/or your team know, and/or does Nightwatch configure these differently?

And yeah, I put my recommendations on the bottom, which is for Selenium to match WDIO on Read Timeout, and standardize max redirects to 20. And make everything configurable.

Changing default Page Load Timeout is a more controversial suggestion, but the current value is useless for most people. 😆

@titusfortner
Copy link
Member Author

@jimevans @harsha509 @AutomatedTester @diemol
This was discussed in TLC meeting on 9 August; can you comment / 👍 / 👎 on this proposal?
Let me know if what I wrote doesn't make sense. Thanks.

@diemol
Copy link
Member

diemol commented Aug 10, 2023

I just updated the table above. Java's default read timeout is 180 seconds, not 200.

@diemol
Copy link
Member

diemol commented Aug 10, 2023

👍

@titusfortner
Copy link
Member Author

@diemol I'm not sure we're looking in the same place. I ran tests timing it as part of my investigation. Either way if we go this route, we'll make sure it is changed properly.

@titusfortner
Copy link
Member Author

And yet it doesn't time out until 200 seconds, so something more is happening. I didn't look too deeply into it, but that number isn't the whole story.

@jimevans
Copy link
Member

jimevans commented Aug 14, 2023

@titusfortner Your table is incorrect. The timeout for the HTTP client in the .NET bindings is entirely configurable. Every concrete driver class (ChromeDriver, FirefoxDriver, etc.) has a constructor that takes a TimeSpan argument for the command timeout, and always has. This command timeout is the HTTP client request timeout. I know of no way to customize the number of redirects allowed by the client in .NET, but I'm happy to be proven wrong. I have no preference on whatever the project decides the defaults should be.

@titusfortner
Copy link
Member Author

Yeah, I smooshed both of those into the same column (are both of these values configurable). I spent time in several of the bindings when working on this... I think I saw where it would get changed in .NET but now I'm not sure if my memory is playing tricks on me. 😂

@jimevans
Copy link
Member

jimevans commented Aug 14, 2023

@titusfortner Actually, yes, the max redirects would be set during the creation of the HttpClient class, which is done in HttpCommandExecutor.CreateHttpClient(). You'd set the MaxAutomaticRedirects property of the created HttpClientHandler object created in line 224 of that file. I reiterate I have no preference on whatever the project decides defaults should be, though I think exposing an API similar to the command timeout would be a bad API design.

@titusfortner
Copy link
Member Author

Yes, that looks like what I saw.

What Java has done, which I like, is create a ClientConfig class that manages things like connection timeout, read timeout, proxies, max redirects, etc. It would be nice to have a similar interface across the bindings (or at least equivalent functionality).

The general idea is that Options class has session related things, Service class has driver related things and ClientConfig class has connection related things. I don't want to see the driver constructors blow up. 😄

@AutomatedTester
Copy link
Member

The tests I did, I couldn't get JS to time out, and I gave up after a bit. @AutomatedTester do you and/or your team know, and/or does Nightwatch configure these differently?

Configured through our config file

@titusfortner
Copy link
Member Author

@AutomatedTester does it do it in a way that we can apply to SeleniumJS, or is it something that needs to be subclassed/??

@titusfortner
Copy link
Member Author

We decided to do this, now we just need to do this.

@joerg1985
Copy link
Member

As mentioned in #12975, it might be good to have a default value which will not conflict with the default w3c page load timeout of 300s. Otherwise there are situations like #13251 even when using the default configuration.

And in general the timeouts of the different grid compoments should be aligned, otherwise the client will timeout with a none speaking message, some ms before the driver would respond with a propably more speaking error message.
Or the grid will still try to create a session, the client did allready reached its timeout and after some timeouts the grid will be blocked with unused sessions until the session timeout is reached.

So in my mind this makes sense to have these conditions for the timeouts (> is 'must be bigger than'):

  • client timeout > router to node timeout > node to driver timeout > w3c specified timeouts
  • client timeout > session request timeout

@titusfortner
Copy link
Member Author

@joerg1985 what are the current values of:

  • router to node timeout
  • node to driver timeout
  • session request timeout

and by

w3c specified timeouts

Do you mean more than just page load timeout?

@joerg1985
Copy link
Member

@titusfortner i was not sure if there where more/higher w3c timeouts, but it seems like the page load is the highest one:

  • Script timeout 30,000 ms
  • Page load timeout 300,000 ms
  • Implicit wait timeout 0 ms

The current defaults are:

So the current timeouts might lead to issues when the client has to wait for new session until the grid has a free slot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants