-
Notifications
You must be signed in to change notification settings - Fork 1
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
register events to avoid processing duplicate events #7
base: main
Are you sure you want to change the base?
Conversation
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.
The change prevents intentional resending of events, e.g., when developing the bot. Maybe it’s good enough as a temporary workaround.
In any case, I would not hardcode the number of threads to one. I think it would be ok to make it a parameter here and for the bot.
@@ -37,4 +37,5 @@ def handle_issue_comment_event(self, event_info, log_file=None): | |||
if __name__ == '__main__': | |||
app = create_app(klass=ExamplePyGHee) | |||
log("App started!") | |||
waitress.serve(app, listen='*:3000') | |||
# stick to single thread so we can avoid processing duplicate event deliveries multiple times | |||
waitress.serve(app, listen='*:3000', threads=1) |
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 wouldn’t do that. It could result in the bot becoming unresponsive if the processing of an event takes a long time.
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.
Short-term, this is a necessary evil, otherwise the detection of duplicate events can't work, that relies on serially processing events...
By default, threads
is 4
with waitress, and to be honest I don't expect big impact of this, since processing a single event shouldn't take much longer than a couple of seconds.
That's true, but I don't see an easy way around that... For a development scenario just restarting the bot would be enough, since the registered events are only kept in memory. |
Just chiming in, I've tried running this version of PyGHee on the bot instance at RUG and did a "lame" stress test by sending a bunch of bot commands in quick succession here: Neves-Bot/software-layer#42 I did see waitress warnings in the
|
@trz42 How shall we proceed here? Merge as is, and check if there's trouble due to the changes (on the |
No description provided.