Skip to content

Conversation

Shastick
Copy link
Contributor

@Shastick Shastick commented Jul 22, 2025

This is a fix for #1078, which should allow uss_qualifier to proceed without crashing out when mime-type errors occur, while keeping around request details for possible troubleshooting.

@Shastick Shastick force-pushed the fix-concurrent-queries-mime-type-exception branch from 64dd4f3 to 0fb4b90 Compare July 22, 2025 11:56
@Shastick Shastick marked this pull request as ready for review July 22, 2025 12:56
@Shastick Shastick force-pushed the fix-concurrent-queries-mime-type-exception branch from 0fb4b90 to 54d994e Compare July 22, 2025 14:34
Copy link
Contributor

@mickmis mickmis left a comment

Choose a reason for hiding this comment

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

I don't think it is desirable to introduce such specific work-around in the low-level class AsyncUTMTestSession, moreover it has not been requested in #1078. In addition it is quite valid for AsyncUTMTestSession to let bubble up any (aiohttp) exception raised during a request as we might want to handle that up the stack. Including a content type exception.

Since scenario HeavyTrafficConcurrent is the only significant caller of AsyncUTMTestSession (except from the prober as well, which we can ignore here), it is IMO enough to intercept the exception there (and be more specific: aiohttp.ClientError is probably a good candidate rather than the generic Exception).

That would be enough to address #1078 without having to come back to it later. And simpler. WDYT?

@Shastick
Copy link
Contributor Author

Shastick commented Aug 12, 2025

Since scenario HeavyTrafficConcurrent is the only significant caller of AsyncUTMTestSession (except from the prober as well, which we can ignore here), it is IMO enough to intercept the exception there (and be more specific: aiohttp.ClientError is probably a good candidate rather than the generic Exception).

What bothers me with the 'just catch any exception from the future awaiting on all the coroutines' approach is that we lose any detail relevant to the request that was made. If we are happy with discarding the request, an approach we can use is to have return_exceptions=True passed to gather, which will return an exception instead of a result for any failing coroutine. At least we can record the details of the successful queries.

Though, in an ideal world uss_qualifier is able to show the request that was made for a failed response, as it may prove useful users.

edit: I can reach the desired effect without the changes in AsyncUTMTestSession by a few changes in the boiler-plate within HeavyTrafficConcurrent (catching the exception at a lower level and returning available details)

@Shastick Shastick force-pushed the fix-concurrent-queries-mime-type-exception branch from 54d994e to e80175d Compare August 12, 2025 13:21
@Shastick Shastick requested a review from mickmis August 12, 2025 13:23
@Shastick
Copy link
Contributor Author

@mickmis the present solution is local to the problematic scenario, while it still keeps as much information as possible about the failed request.

Copy link
Contributor

@mickmis mickmis left a comment

Choose a reason for hiding this comment

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

@mickmis the present solution is local to the problematic scenario, while it still keeps as much information as possible about the failed request.

Neat solution. Thanks!

@mickmis mickmis merged commit 8910847 into interuss:main Aug 14, 2025
21 checks passed
@mickmis mickmis deleted the fix-concurrent-queries-mime-type-exception branch August 14, 2025 13:54
github-actions bot added a commit that referenced this pull request Aug 14, 2025
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.

2 participants