-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 better validation around matching response with request information #3140
Conversation
- Add better validation that the cached request information really matches the response we are trying to process. This is done by passing the response id to get the cached request information for the matching id. This resolves an issue reported on Discord where ``await asyncio.gather(one_request, second_request)`` was guessing the wrong request id for each request since they are basically being triggered / yielding to each other in a shared context.
- asyncio.gather() will run all tasks, yielding to each other. This makes for a good test case to test that each request information that is cached is matched appropriately to the response that is received and its id.
request_id = next(copy(self._provider.request_counter)) - 1 | ||
cache_key = generate_cache_key(request_id) | ||
current_request_cached_info: RequestInformation = ( | ||
cache_key = generate_cache_key(response_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the examples we were walking through earlier, I think there was a sticky point that some responses didnt appear to include an id
. If that happens, generate_cache_key
will raise. Should we swallow that exception and just skip the processors here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. We could still return the raw response and try to not halt things here. I think passing in the response
to this method would allow us to retrieve the id
if it's there or debug log the response that doesn't have an id
and keep moving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that sounds like it would work great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
What was wrong?
response_id
for the matching request once we go back through the middleware onion and the response is present. This area of the code hadn't been iterated on since some of the earlier commits in the building out of the provider. This is a very important and necessary improvement onid
matching.This resolves an issue reported on Discord where
await asyncio.gather(one_request, second_request)
was guessing the wrong request id for each request since they are basically being triggered / yielding to each other in a shared context.Steps to reproduce
Todo:
Cute Animal Picture