fix: dont wake up calloop if no messages were read#9
Conversation
| if let Some(Err(WaylandError::Io(err))) = guard.map(ReadEventsGuard::read) { | ||
| // If some other thread read events before us, concurrently, that's an expected | ||
| // case, so this error isn't an issue. Other error kinds do need to be returned, | ||
| // however | ||
| if err.kind() != io::ErrorKind::WouldBlock { | ||
| match guard.map(ReadEventsGuard::read) { | ||
| Some(Ok(n)) => { | ||
| self.messages_read = n; | ||
| }, | ||
| Some(Err(WaylandError::Io(err))) => { | ||
| // If some other thread read events before us, concurrently, that's an expected | ||
| // case, so this error isn't an issue. Other error kinds do need to be returned, | ||
| // however | ||
| if err.kind() != io::ErrorKind::WouldBlock { | ||
| // before_handle_events doesn't allow returning errors | ||
| // For now, cache it in self until process_events is called | ||
| self.stored_error = Err(err); | ||
| } | ||
| }, | ||
| Some(Err(WaylandError::Protocol(err))) => { | ||
| log_error!("Protocol error received on display: {}", err); | ||
| // before_handle_events doesn't allow returning errors | ||
| // For now, cache it in self until process_events is called | ||
| self.stored_error = Err(err); | ||
| } |
There was a problem hiding this comment.
I found that this if let is not exhaustive. I can split this into a separate commit if you prefer. Or we can discard it completely if it does not make sense
kchibisov
left a comment
kchibisov
left a comment
There was a problem hiding this comment.
Have you read this big comment?
// If some other thread read events before us, concurrently, that's an expected
// case, so this error isn't an issue. Other error kinds do need to be returned,
// however
You clearly broke the dispatch logic, because if some other thread enqueued events there won't be any events for us to read, thus Read(0) but we can have some enqueued by other thread in MT environment (read as using any graphics driver).
I'm not sure we can how many events are there in our queue though, maybe if we can check that there're none in our queue we can avoid dispatch logic.
You are right, thanks. My bad, I had not thought through that. |
2 participants
I found that my event loop was being woken up twice back to back by
WaylandSource. Turns out the socket'sread()call can returnOk(0)for some reason. My guess is that the socket becomes readable but no complete events are found on it.In such case, maybe it's best to skip dispatching, thus not waking up the event loop into which this source is inserted.
It is entirely possible that the extra check is not worth the effort. Maybe there are better ways to solve this. Maybe this is just not an issue.
I have no strong opinions or deep knowledge in this regard, so feel free to close this PR or modify it however you see fit if it's not applicable.