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

add client_wait_for_response option to some method calls #222

Merged
merged 6 commits into from
Sep 13, 2019

Conversation

risicle
Copy link
Contributor

@risicle risicle commented Aug 30, 2019

This is a part of a potential way of addressing https://trello.com/c/LABe8BKA

The pattern it introduces is something I like to call async-not-async. It's simply allows a user of the apiclient to perform a highly simplified form of async http request by ignoring the response from the server. It does this by setting the read timeout to something tiny and allowing that timeout to happen. It relies on the fact that web server stacks tend not to readily propagate "the client is gone" information, and a request will continue to be processed fully, even after the client has given up.

In the case we're proposing to use it it gives us basically all the advantages of true async but also all the perils: how easy it is to launch hundreds of requests in a couple of milliseconds without considering how the recipient will deal with it. Pushed too far, what we'd effectively end up doing is using our http servers' connection buffers as a not-very-good task queue, which could lead to healthcheck failures.

Why prefix the kwarg with client_? Because previously all arguments passed to apiclient methods implied they were referring to server options. This is the first relating to client behaviour, and the distinction could start to get ambiguous if and when we start doing things like adding a server-side option to the api's update_service endpoint to instruct it to perform its daisychained index request to the search-api without waiting for the response.

@risicle
Copy link
Contributor Author

risicle commented Sep 2, 2019

I've also added the wait_for_index option to DataAPIClient.update_service() which in turn uses the above, wait_for_response, option (inside the api's view).

@risicle
Copy link
Contributor Author

risicle commented Sep 3, 2019

Ok, this does not work IRL, though I think I know what I've got to do to fix it, so don't bother testing quite yet...

@benvand
Copy link
Contributor

benvand commented Sep 4, 2019

What commands are we going to issue as client_wait_for_response in production?

@risicle
Copy link
Contributor Author

risicle commented Sep 4, 2019

Currently the only one planned is SearchAPIClient.index(), but had there been an easy existing mechanism to add is as a common option across all calls, I might have added it to everything. But there wasn't.

@risicle
Copy link
Contributor Author

risicle commented Sep 4, 2019

Ok it works now, but when used in this way it emits a bunch of log events to the effect that the request failed. Which it kinda didn't. I'll see what I can do.

@risicle risicle force-pushed the ris-ignore-response branch 6 times, most recently from a2e1fe8 to 033711c Compare September 5, 2019 13:46
@risicle
Copy link
Contributor Author

risicle commented Sep 5, 2019

This works "fully" now, only through a lot of investigation into what exceptions requests really lets through to the caller. Short answer: it varies. And a lot of them wrap other exceptions in slightly non-standard ways so I've had to build a method _iter_exceptions_by_cause to find these exceptions in the most flexible way possible.

@katstevens
Copy link
Contributor

I will try and make some time today to test this out locally (with the corresponding API branch).

Copy link
Contributor

@katstevens katstevens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - have tested this locally, let's try it 👍

@risicle risicle merged commit 52aca27 into master Sep 13, 2019
@risicle risicle deleted the ris-ignore-response branch September 13, 2019 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants