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

source-braintree-native: concatenate search limit error message #2193

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

Alex-Bair
Copy link
Contributor

@Alex-Bair Alex-Bair commented Dec 10, 2024

Description:

The Braintree SDK's API calls are all synchronous, so they block the event loop, meaning only a subset of streams can make progress at a time. Adding await asyncio.sleep(0) to the top of the various fetch functions allows other streams to make progress since these functions now yield to the event loop.

Using await asyncio.sleep(0) within the connector-specific code for this purpose is a temporary measure until I refactor the connector to make asynchronous API requests.

I also fixed an issue with the search limit error message where the f-strings were not concatenated together & the full error message wasn't getting displayed.

Workflow steps:

(How does one use this feature, and how has it changed)

Documentation links affected:

(list any documentation links that you created, or existing ones that you've identified as needing updates, along with a brief description)

Notes for reviewers:

(anything that might help someone review this PR)


This change is Reviewable

@Alex-Bair Alex-Bair added the change:unplanned Unplanned change, useful for things like doc updates label Dec 10, 2024
@@ -93,6 +99,8 @@ async def fetch_transactions(
log: Logger,
log_cursor: LogCursor,
) -> AsyncGenerator[IncrementalResource | LogCursor, None]:
# Yield to the event loop to prevent starvation.
await asyncio.sleep(0)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is going to do what you're after, which I assume is to have multiple calls to the Braintree API in progress at a time, concurrently. The calls to braintree_gateway.transaction etc. are not async, so they will still block the event loop. I may be mis-reading / mis-interpreting what you're after here though, and adding this isn't going to break anything, so just FYI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, this isn't intended to try and let multiple calls progress concurrently. I won't be able to achieve that without making the API calls asynchronous. My main concern was that if a stream isn't caught up to the present, it would continue trying to get caught up by immediately reinvoking fetch_foobar & blocking any of the other streams from making API calls since there aren't any places within fetch_foobar that yields to the event loop. I also thought this might prevent the SIGQUIT mechanism from working if one of the streams was trying to catch up to the present. But I tested locally, and sending a SIGQUIT still works without these await asyncio.sleep(0) lines, so there's still an await in the CDK that's yielding to the event loop and preventing these issues (I think it's here).

I'll update the PR to remove these await asyncio.sleep(0) lines since they aren't needed. I'll keep the error message fix and merge that.

Copy link
Member

@williamhbaker williamhbaker left a comment

Choose a reason for hiding this comment

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

LGTM

@Alex-Bair Alex-Bair force-pushed the bair/source-braintree-native-misc-fixes branch from e33cc52 to 8f3805c Compare December 10, 2024 15:52
@Alex-Bair Alex-Bair changed the title source-braintree-native: make fetch functions non-blocking source-braintree-native: concatenate search limit error message Dec 10, 2024
@Alex-Bair Alex-Bair merged commit a6654f0 into main Dec 10, 2024
73 of 79 checks passed
@Alex-Bair Alex-Bair deleted the bair/source-braintree-native-misc-fixes branch December 10, 2024 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change:unplanned Unplanned change, useful for things like doc updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants