Skip to content
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

The callbacks API is unsound and the event sender/receiver can potentially deliver events out of order #584

Open
aloucks opened this issue Jan 14, 2025 · 1 comment

Comments

@aloucks
Copy link
Contributor

aloucks commented Jan 14, 2025

The PRs below are where most of the issues were introduced. In addition to the issues in the title, the API now returns PWindow instead of Window with erroneous unsafe Send and Sync implementations. Most Window operations must be called from the main thread due to platform restrictions. This is documented in the GLFW docs. (example: https://www.glfw.org/docs/3.3/group__window.html#ga5d877f09e968cef7a360b513306f17ff)

Window Callbacks: this API create multiple mutable aliases to Windows. The original event API was structured intentionally because it's unsafe and unsound to implement the callbacks this way.
#551

Events: The std mpsc channel was replaced with something hand rolled, which can deliver events out of order if the VecDeque becomes full.
#552

Example timeline of how this could happen:
event thread -> push 256th event into VecDeque
event thread -> put 257th event on secondary channel (because VecDeque is full)

reader thread -> pull 1st event from VecDeque
event thread -> push 258th event onto VecDeque (because it has one slot free)
reader thread -> pull events 2-256 event from VecDeque
reader thread -> pull event 258 from VecDeqeue (it prioritizes the VecDeque over the secondary channel)

The 257the event won't be read from the channel as long as new events are going into the VecDeque

@bvssvni I'd like to work on reverting some of this and cleaning things up, but I wanted to gauge your appetite for this before starting.

@bvssvni
Copy link
Member

bvssvni commented Jan 21, 2025

@aloucks feel free to go ahead!

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

No branches or pull requests

2 participants