backend: fix prepare_read behavior

[elinorbgr]

This should fix the multithreading issues that have been encountered.

cc @kchibisov

[codecov]

Codecov Report

Patch coverage: 76.47% and project coverage change: +0.02% 🎉

Comparison is base (952820c) 73.09% compared to head (e0372d0) 73.11%.

❗ Current head e0372d0 differs from pull request most recent head 69eae18. Consider uploading reports for the commit 69eae18 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #650      +/-   ##
==========================================
+ Coverage   73.09%   73.11%   +0.02%     
==========================================
  Files          47       47              
  Lines        7711     7691      -20     
==========================================
- Hits         5636     5623      -13     
+ Misses       2075     2068       -7     

Flag	Coverage Δ
main	58.59% <52.94%> (+0.03%)	⬆️
test--	78.09% <77.77%> (+<0.01%)	⬆️
test--server_system	61.29% <77.77%> (-0.02%)	⬇️
test-client_system-	69.12% <64.70%> (+0.01%)	⬆️
test-client_system-server_system	51.31% <64.70%> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
wayland-backend/src/rs/wire.rs	93.33% <ø> (ø)
wayland-client/src/event_queue.rs	58.92% <50.00%> (+0.33%)	⬆️
wayland-backend/src/sys/client_impl/mod.rs	72.29% <75.00%> (+0.23%)	⬆️
wayland-client/src/conn.rs	85.83% <75.00%> (-0.35%)	⬇️
wayland-backend/src/client_api.rs	90.47% <100.00%> (ø)
wayland-backend/src/rs/client_impl/mod.rs	78.94% <100.00%> (ø)

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kchibisov]

Probably the same about #[must_use] for other methods. Also, maybe we should use Result instead, but use some io type to indicate that lock can't be acquired? The mutex stuff uses Result and it's more intuitive than the Option.

[kchibisov]

Hm, but you could have a blocking dispatch of the queue here as well? Non-blocking roundtrip doesn't sound like it'll work reliably or I don't understand something?

[elinorbgr]

This is in a loop, so it'll just try again at the next iteration.

[kchibisov]

Ah, yeah, sure, I just know that there's mo efficient API to do the exact same thing, but I think it's not an option for us. The wl_display_dispatch_queue https://wayland.freedesktop.org/docs/html/apb.html#Client-classwl__display_1ae027b09801474ac7c6b0f1ef25ff6e17

[elinorbgr]

That function in wayland-client does mostly exactly the same thing as wl_display_roundtrip, but implemented in terms of wayland-backend's API, so I don't really get your point?

[kchibisov]

wl_display_roundtrip uses poll under the hood to not do the loop.

[elinorbgr]

What... ?

wl_display_roundtrip does have a loop: https://gitlab.freedesktop.org/wayland/wayland/-/blob/main/src/wayland-client.c#L1402

and blocking_read uses poll internally: https://github.com/Smithay/wayland-rs/blob/master/wayland-client/src/conn.rs#L224-L245

[kchibisov]

@elinorbgr if you look closely what their code is and what we have, they use blocking dispatch. However we use it only in read_guard branch, and the other branch is using regular non-blocking dispatch.

So while they have a loop, the wait, but we busy loop from the else. The blocking read is fine, the else is a busy loop. dispatch_inner_queue is a non_blocking API from what I can, see, but libwayland always using blocking API, with dispatch_queue.

[kchibisov]

After reading the libwayland impl it's pretty much the same, when their docs are not really clear what exactly blocks and how it works.

[elinorbgr]

Also, maybe we should use Result instead, but use some io type to indicate that lock can't be acquired?

Not sure what we would gain from this? It feels to me that this Option<_> reflects best the behavior of this method, changing it to a Result we would just have an error type identical to (), what would be the point then?