-
Notifications
You must be signed in to change notification settings - Fork 4
ctx.run
Blocks Event Loop for Sync Functions
#46
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
Comments
To consistently reproduce the bad behavior (sequential execution), use:
instead. Replacing |
Hi @ouatu-ro, ctx.run("customer id" , lambda : uuid() ) Having to force this using Perhaps we need to insert a variant to the |
Hi @igalshilman, Sorry, I wasn’t clear earlier. Your suggestion is exactly what I tired. Running actions in a background thread by default makes most sense for Python. I modified the implementation of if inspect.iscoroutinefunction(action):
action_result = await action() # type: ignore
else:
action_result = action() # performance killer in an async context to: if inspect.iscoroutinefunction(action):
action_result = await action() # type: ignore
else:
action_result = await asyncio.to_thread(action) This still works great with This change allows synchronous functions to run without blocking the event loop which is crucial for maintaining performance of other handlers. Currently all handlers get affected by sync executions inside Instead of always using What's more, since I believe this change is crucial. For example, consider this scenario: with the current @greeter.handler()
async def sleep_handler(ctx: Context, secs: int):
await ctx.run("waiting", lambda: time.sleep(secs))
return f"time.sleep {secs} seconds"
@greeter.handler()
async def greet(ctx: Context, req: GreetingRequest) -> Greeting:
promises = [ctx.service_call(sleep_handler, arg=5) for _ in range(20)]
start_time = time.time()
results = [await promise for promise in promises]
end_time = time.time()
print(f"{end_time - start_time=}") With the current ctx.run implementation, running even 10 handler invocations causes a significant performance hit across all Restate handlers. Offloading sync actions to a background thread should mitigate that. What do you think? |
Hey @ouatu-ro ! |
ctx.run
does not properly handle synchronous functions, running them directly in the event loop instead of offloading them. This causes inconsistent behavior:Steps to Reproduce
Run the following code and start two handler invocations at the same time:
Expected Behavior
ctx.service_call
should allow both tasks to execute in parallel, finishing in ~5 seconds.Actual Behavior
ctx.run
blocks for 5 seconds, then the second runs for another 5 seconds.Example Output (Sequential Execution):
Possible Root Cause
Currently,
ctx.run
executes sync functions inline:This prevents efficient execution of multiple
ctx.run
calls.Proposed Fix
Modify
ctx.run
to automatically offload sync functions:Impact
ctx.run
calls to execute concurrently.Would love to hear thoughts on this!
Restate version: 0.5.1
The text was updated successfully, but these errors were encountered: