test: Fix intermittent failures with ReqwestClient #262
Conversation
| @@ -20,10 +20,13 @@ pub struct ReqwestClient { | |||
|
|
|||
| impl ReqwestClient { | |||
| /// Instantiate a new Reqwest client to perform HTTP requests. | |||
| pub fn new() -> ReqwestClient { | |||
| Self { | |||
| reqwest_client: reqwest::Client::new(), | |||
There was a problem hiding this comment.
Just noticed that request::Client::new() could panic in certain cases, but the builders let you capture those errors without panicking.
| let reqwest_client = reqwest::Client::builder() | ||
| // Disable the connection pool to avoid the IncompleteMessage errors. | ||
| // See #259 for more details. | ||
| .pool_max_idle_per_host(0) |
There was a problem hiding this comment.
I'm wondering why this wasn't happening with the other implementation: we were not setting the max idle per host there as well. Any idea?
Aside from that, I don't think there's a big problem in disabling pooling.
There was a problem hiding this comment.
I think a better thing to do would be to change the time that reqwest keeps connections around so it doesn't outlive the Kinto connections, but I can't find what that value would be. After that, I think adding retry is going to be an important long term solution for many parts of Merino. Neither of these are quick though.
This unblocks us now, and is simple enough to understand. Lets file an issue to come back to this, and merge this fix.
|
I'll note that hyper v0.14.3 has a fix around connection reuse + flushing but it definitely does not help, I tried it as a long shot here |
* Revert "Merge pull request #232 from Dexterp37/rs-client" - Integrate remote-settings-client * Revert "Merge pull request #260 from mozilla-services/kinto-url-with-v1" - Include `/v1` on the end of the remote settings URL we give to the RS client * Revert "Merge pull request #261 from mozilla-services/feat/259" don't hide inner errors in the log message * Revert "Merge pull request #262 from mozilla-services/gh-259" Fix intermittent failures with ReqwestClient
This fixes #259.
The issue was caused by the connection pool in Reqwest/Hyper. See more details in here.
Since
ReqwestClientis only used by RS suggester, and it is not making constant network calls to RS. Disabling the connection pool should not be a big deal, but I will let @mythmon @Dexterp37 decide if this is acceptable or shall we keep it as is and add the retry to the API.