register events to avoid processing duplicate events

[boegel]

No description provided.

[trz42]

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.

[trz42]

I wouldn’t do that. It could result in the bot becoming unresponsive if the processing of an event takes a long time.

[boegel]

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.

[boegel]

Handling of deploy event may take a while, which could result in some delays in processing other events.

If we obtain a lock in register_event and release it when we're done, forcing a single thread won't be necessary

[boegel]

Hmm, maybe I'm being too careful here, because of the Python GIL (https://docs.python.org/3/glossary.html#term-global-interpreter-lock)?

So maybe I don't need to restrict this to a single thread at all even though multiple threads can update global state (the self.registered_events list)...

[boegel]

The change prevents intentional resending of events, e.g., when developing the bot. Maybe it’s good enough as a temporary workaround.

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.

[Neves-P]

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 event_handler but the impact was minimal, I couldn't really tell tasks were waiting for others to finish just by looking at the PR's comments. That said, this was just me messing around in one PR, the traffic on the EESSI repo is higher of course. Nonetheless, even if 3 or 4 people were to submit their commands at once, I wonder if we would notice more than a minor slowdown. Then again, I don't know how much the bot can handle waiting for new tasks.

logging in to /home1/f115372/bot/eessi_bot_event_handler.log
WARNING:waitress.queue:Task queue depth is 1
WARNING:waitress.queue:Task queue depth is 2
WARNING:waitress.queue:Task queue depth is 3
WARNING:waitress.queue:Task queue depth is 4
WARNING:waitress.queue:Task queue depth is 5

[boegel]

@trz42 How shall we proceed here?

Merge as is, and check if there's trouble due to the changes (on the dev.eessi.io bot for example)?

[ocaisa]

The locking can be done here according to ChatGPT

...
import threading

...
# Thread lock for safe access
lock = threading.Lock()
...
with lock:
    self.registered_events.append(event_id)

[ocaisa]

import threading

# Create a lock once and reuse it
lock = threading.Lock()

def safe_function():
    with lock:  # Reuses the same lock
        # Critical section (thread-safe code)
        print("Thread-safe operation")

[boegel]

Locking added, so no more need to stick to a single-thread.

I'll get this merged, so we can battle-test this, but should work...

requested change made, no longer using single-threaded run