-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix: do not block entire program with sleep #1374
fix: do not block entire program with sleep #1374
Conversation
Signed-off-by: San Nguyen <[email protected]>
Signed-off-by: San Nguyen <[email protected]>
Signed-off-by: San Nguyen <[email protected]>
Signed-off-by: San Nguyen <[email protected]>
Why is this sleep statement there in the first place? @willydouhard |
created_at, | ||
) | ||
return self | ||
|
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.
I don't see where this init()
is called, here or anywhere. Could you please explain why this is here?
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.
Also note the type error raised by mypy.
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.
@dokterbob , as mentioned, if sleep is needed then we will need to initiate all Message or Step with init
, which will be lots of changes. May I confirm on your side that sleep is absolutely needed before making a big change?
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.
I don't know why the sleep statement is here. Perhaps @willydouhard does?
I am asking about init() because you add this code to this PR and it does not seem to be called anywhere. I'd like to avoid dead code as much as possible.
Hi, is there any update so we can just remove the sleep statement? |
hi @willydouhard , can we have your comment? |
This is a hacky way of making sure the start date of a step is < than the end date. Not possible to use async version everywhere as of know since we use it in the constructor of the Message class (which can't be async). I'd like to see this go away. Will think about it! |
For now I merged this #1634 |
Signed-off-by: San Nguyen [email protected]
with time.sleep(), the entire program is stopped, which yield very bad performance for server handling concurrent requests.
Introducing new
async init
method for creating Step and Message withasyncio.sleep
so that the async loop will give back the control to the program while waiting for sleep.Let me know what is your thought and I help to replace existing usage with the new one.