Skip to content

fix(server): dispatch callbacks and queue events - #856

Open
gijzelaerr wants to merge 1 commit into
masterfrom
fix/server-event-callbacks
Open

fix(server): dispatch callbacks and queue events#856
gijzelaerr wants to merge 1 commit into
masterfrom
fix/server-event-callbacks

Conversation

@gijzelaerr

Copy link
Copy Markdown
Owner

Summary

  • dispatch native-compatible lifecycle, client, read, and write server events
  • deliver events through both callbacks and the pick_event queue
  • make queue access thread-safe and isolate callback failures from the server loop
  • correct the startup event code to evcServerStarted

Validation

  • uv run --frozen pre-commit run --all-files
  • pytest -q (1822 passed, 78 skipped)
  • uv build --no-sources

Addresses #853.

@NiklasReisser NiklasReisser left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

To be honest, I don't fully understand the way pick_event is supposed to be used.
Should I busy wait in my application?
Or rather call pick_event in my callback to prevent the queue from overflowing?

Comment thread tests/test_server.py
self.assertIsInstance(read_event, SrvEvent)
self.assertIsInstance(write_event, SrvEvent)
assert isinstance(read_event, SrvEvent)
assert isinstance(write_event, SrvEvent)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why the duplicated check? Wouldn't it be more important to check the local events list filed by the callback?

Comment thread snap7/server/__init__.py

# Event queue for pick_event
self._event_queue: List[SrvEvent] = []
self._event_lock = threading.Lock()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider using https://docs.python.org/3/library/queue.html to prevent the list from filling up too much. Also, you wouldn't need the lock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants