Update dependencies and add an example #1

DJMcNab · 2023-09-13T18:19:57Z

This now includes the changes in Smithay/calloop#144

This should be sufficient to fix some of the weirdness that
winit experiences, as well as resolve the hangs that Glazier finds

src/lib.rs

kchibisov

Thanks, for working on it. Some nits.

src/lib.rs

kchibisov · 2023-09-14T18:31:15Z

src/lib.rs

+            // Wayland-backend should probably document some guarantees here to make this sound,
+            // but this is unlikely to change


the last part is not clear, what is unlikely to change?

src/lib.rs

kchibisov · 2023-09-14T18:32:38Z

src/lib.rs

+        // In theory, we would need to call the process_events handler on fd. However,
+        // we know that Generic's process events's call is a no-op, so we can just handle the event ourselves


I find it hard to understand what you want to say with this comment, since it's not attached to anything.

src/lib.rs

kchibisov · 2023-09-14T18:36:27Z

src/lib.rs

    read_guard: Option<ReadEventsGuard>,
+    fake_token: Option<Token>,


We should state what and why it's here.

kchibisov · 2023-09-14T18:37:47Z

src/lib.rs

+            // If getting the guard failed, that means that there are
+            // events in the queue, and
+            // The readiness value is never used, we just need some marker
+            // If getting the guard failed, we need to process the events 'instantly'
+            // tell calloop this


The formatting of the comments on the patch seems off in general. Could you properly format them?

kchibisov · 2023-09-14T18:39:17Z

src/lib.rs

+        // Otherwise, drop the guard if we have it, as we don't want to do any
+        // reading when we didn't get any events


it's always dropped? There's no otherwise.

Well it's always dropped, sure. But it's not always dropped by us - if the read call happens, that takes ownership and drops it.

If we call the read it still sort of dropped by us. The comment is a bit misleading imho, because read should take ownership, no?

kchibisov · 2023-09-14T18:40:39Z

src/lib.rs

+            if let Some(guard) = guard {
+                if let Err(WaylandError::Io(err)) = guard.read() {


You can probably merge these two and probably, but I don't really care.

I don't really see how without let chains, which I don't think are stable yet

I've suggested in the diff how to merge them.

I see. It's not the code style I would use, but I'll apply it anyway.

I mean, it's less nested and that's how usually written. More readable code should be preferred.

src/lib.rs

kchibisov · 2023-09-16T11:12:30Z

Looks good from what I can say. Some formatting diff, which you may want to adjust.

diff --git a/src/lib.rs b/src/lib.rs
index 8527e18..3384192 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -79,11 +79,11 @@ impl<D> WaylandSource<D> {
     pub fn new(queue: EventQueue<D>) -> Option<WaylandSource<D>> {
         let guard = queue.prepare_read()?;
         let fd = Generic::new(
-            // Safety: `connection_fd` returns the wayland socket fd,
-            // and EventQueue (eventually) owns this socket
-            // fd is only used in calloop, which guarantees
-            // that the source is unregistered before dropping it, so the
-            // fd cannot be used after being freed
+            // SAFETY: `connection_fd` returns the wayland socket fd, and
+            // EventQueue (eventually) owns this socket fd is only used in
+            // calloop, which guarantees that the source is unregistered before
+            // dropping it, so the fd cannot be used after being freed
+            //
             // Wayland-backend should probably document some guarantees here to make this sound,
             // but we know that they are unlikely to make the queue have a different socket/fd
             // - there is no public API to do so
@@ -145,10 +145,10 @@ impl<D> EventSource for WaylandSource<D> {
         // Take the stored error
         std::mem::replace(&mut self.stored_error, Ok(()))?;
 
-        // Because our polling is based on a `Generic` source, in theory we might want
-        // to to call the process_events handler on fd. However,
-        // we know that Generic's `process_events` call is a no-op, so we can just
-        // handle the event ourselves
+        // NOTE: Because our polling is based on a `Generic` source, in theory we might
+        // want to to call the process_events handler on fd. However, we know
+        // that Generic's `process_events` call is a no-op, so we can just
+        // handle the event ourselves.
 
         // Dispatch any pending events in the queue
         Self::loop_callback_pending(queue, &mut callback)?;
@@ -188,33 +188,31 @@ impl<D> EventSource for WaylandSource<D> {
         self.read_guard = self.queue.prepare_read();
         match self.read_guard {
             Some(_) => Ok(None),
-            // If getting the guard failed, that means that there are
-            // events in the queue, and so we need to handle the events instantly
-            // rather than waiting on an event in polling. We tell calloop this
-            // by returning Some here. Note that the readiness value is
-            // never used, so we just need some marker
+            // If getting the guard failed, that means that there are events in the queue, and so
+            // we need to handle the events instantly rather than waiting on an event in polling.
+            // We tell calloop this by returning Some here. Note that the readiness value is never
+            // used, so we just need some marker.
             None => Ok(Some((Readiness::EMPTY, self.fake_token.unwrap()))),
         }
     }
 
     fn before_handle_events(&mut self, events: calloop::EventIterator<'_>) {
-        // It's important that the guard isn't held whilst process_events calls occur
+        // It's important that the guard isn't held whilst process_events calls occur.
+        //
         // This can use arbitrary user-provided code, which may want to use the wayland
-        // socket For example, creating a Vulkan surface needs access to the
-        // connection
+        // socket. For example, creating a Vulkan surface needs access to the
+        // connection.
         let guard = self.read_guard.take();
         if events.count() > 0 {
             // Read events from the socket if any are available
-            if let Some(guard) = guard {
-                if let Err(WaylandError::Io(err)) = guard.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 {
-                        // before_handle_events doesn't allow returning errors
-                        // For now, cache it in self until process_events is called
-                        self.stored_error = Err(err);
-                    }
+            if let Some(Err(WaylandError::Io(err))) = guard.map(|guard| guard.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 {
+                    // before_handle_events doesn't allow returning errors
+                    // For now, cache it in self until process_events is called
+                    self.stored_error = Err(err);
                 }
             }
         }
@@ -223,16 +221,16 @@ impl<D> EventSource for WaylandSource<D> {
 
 fn flush_queue<D>(queue: &mut EventQueue<D>) -> Result<(), calloop::Error> {
     if let Err(WaylandError::Io(err)) = queue.flush() {
+        // WouldBlock error means the compositor could not process all our messages
+        // quickly. Either it is slowed down or we are a spammer. The error is
+        // not critical on its own and we should simply retry later.
         if err.kind() != io::ErrorKind::WouldBlock {
             // in case of error, forward it and fast-exit
             log_error!("Error trying to flush the wayland display: {}", err);
             return Err(err.into());
         }
-        // WouldBlock error means the compositor could not process all
-        // our messages quickly. Either it is slowed
-        // down or we are a spammer. Should not really
-        // happen, if it does we do nothing and will flush again later
     }
+
     Ok(())
 }

kchibisov

The patch unfortunatelly doesn't work and busy loops inside the new calloop logic.

kchibisov · 2023-09-17T16:12:07Z

With the calloop's patch sctk works now. Were you able to verify that your project works as well and the issues are really resolved?

I'm not sure about the example here, since it adds extra maintainance and the calloop wayland source is used within sctk, so it's not that hard to test with it. And I tested with the sctk because it has more complex configurations with the calloop.

kchibisov · 2023-09-17T16:27:52Z

src/lib.rs

        let guard = queue.prepare_read()?;
-        let fd = Generic::new(guard.connection_fd().as_raw_fd(), Interest::READ, Mode::Level);
+        let fd = Generic::new(


Hm, what's the reason we try to acquire the guard when creating the source? I know that it was like that before, but what is the reason, @elinorbgr ? We won't do any polling here, so it's a bit weird, just to check that the FD is alive?

In this repository, it's entirely because wayland-backend chooses not to expose the fd through any mechanism other than the guard, as far as I can tell. The fd should still be valid any time the event queue (and hence the Connection) is around, although we're not given that guarantee.

Indeed, though Smithay/wayland-rs#656 is adding the necessary APIs

Yep, Smithay/wayland-rs#656 lets us do this in a clean and safe way with simply let queue = Generic::new(queue, Interest::READ, Mode::Level);. Then using self.queue.file to access the contained EventQueue.

https://github.com/Smithay/client-toolkit/pull/403/files#diff-8d59229f4c6aea492f0cbceaafc305fcade8dbe513b86e6466feed5a0b2b3107 has a version doing that.

We'd need to wait for a patch release, but I'd rather do so.

DJMcNab · 2023-09-17T16:58:46Z

Not having an example makes for a very poor development experience. If the recommended way to test is to hope that sctk doesn't have conflicting dependencies, and test there, then I would expect the outcome to be that testing won't occur.

Adding the example allows this crate to be tested by itself. It isn't completely necessary, though

kchibisov · 2023-09-17T17:02:37Z

Maybe we should change the way example works by creating one more thread and trying to poll on it as well? So we could try to test the fallback code path? I'm just used to having all the relevant crates around so testing is very simple, but I understand that it might not be the case for everyone.

elinorbgr · 2023-09-19T05:55:59Z

FYI calloop 0.12.1 and wayland-client 0.31.1 are out

kchibisov

And the docs/etc should be resolved.

kchibisov · 2023-09-20T11:28:08Z

src/lib.rs

+    /// `queue` must be from the connection `Connection`.
+    /// This is not a safety invariant, but not following this may cause
+    /// freezes or hangs
+    #[must_use]


must_use is not required anymore.

kchibisov · 2023-09-20T11:29:24Z

src/lib.rs


-            Ok(PostAction::Continue)
-        })?;
+        // Safety: We don't replace the


This comment looks out of place.

kchibisov · 2023-09-20T11:29:44Z

src/lib.rs

+            if let Some(guard) = guard {
+                if let Err(WaylandError::Io(err)) = guard.read() {


I've suggested in the diff how to merge them.

kchibisov · 2023-09-20T11:30:18Z

src/lib.rs

+        // WouldBlock error means the compositor could not process all
+        // our messages quickly. Either it is slowed
+        // down or we are a spammer. Should not really
+        // happen, if it does we do nothing and will flush again later


This comment still seems a bit weird, should be above the WouldBlock error.

This was the existing style, but I'll move it up

kchibisov · 2023-09-20T11:31:56Z

src/lib.rs

@@ -28,8 +28,8 @@
 //! }
 //! ```

+#![forbid(unsafe_op_in_unsafe_fn)]


Suggested change

#![forbid(unsafe_op_in_unsafe_fn)]

#![deny(unsafe_op_in_unsafe_fn)]

DJMcNab added 3 commits September 13, 2023 19:16

Update calloop and wayland-backend

5cce320

Update some docs

1e33943

Run nightly formatting

8e1db02

kchibisov self-requested a review September 14, 2023 09:33

elinorbgr reviewed Sep 14, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

Address review comments

ebeb344

kchibisov requested changes Sep 14, 2023

View reviewed changes

DJMcNab added 5 commits September 14, 2023 20:08

Address review comments

8cdd7dd

Remove dangling 3.

1bd5909

Move flush into its own function

2f79656

Move the explanatory comment

54f4d0d

Do nightly formatting

ee6b605

DJMcNab requested a review from kchibisov September 15, 2023 17:27

kchibisov requested changes Sep 16, 2023

View reviewed changes

kchibisov mentioned this pull request Sep 16, 2023

Update calloop and wayland-rs to the latest versions Smithay/client-toolkit#404

Merged

DJMcNab mentioned this pull request Sep 16, 2023

Fix the additional lifecycle events to work as documented Smithay/calloop#149

Merged

Add an example (reproduced mostly from wayland-client)

b73f07d

DJMcNab changed the title ~~Update dependencies~~ Update dependencies and add an example Sep 16, 2023

kchibisov reviewed Sep 17, 2023

View reviewed changes

Update to latest calloop and wayland-client

8a4034e

kchibisov requested changes Sep 20, 2023

View reviewed changes

DJMcNab added 2 commits September 20, 2023 13:04

Apply suggested changes from review

ab4b6cc

Apply nightly formatting and fix docs

d3d1cf6

kchibisov approved these changes Sep 20, 2023

View reviewed changes

kchibisov merged commit 5dfdd54 into Smithay:main Sep 20, 2023
4 checks passed

DJMcNab deleted the update-deps branch September 20, 2023 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update dependencies and add an example #1

Update dependencies and add an example #1

DJMcNab commented Sep 13, 2023

kchibisov left a comment

kchibisov Sep 14, 2023

kchibisov Sep 14, 2023

kchibisov Sep 14, 2023

kchibisov Sep 14, 2023

kchibisov Sep 14, 2023

DJMcNab Sep 14, 2023

kchibisov Sep 15, 2023

kchibisov Sep 14, 2023

DJMcNab Sep 14, 2023

kchibisov Sep 20, 2023

DJMcNab Sep 20, 2023

kchibisov Sep 20, 2023

kchibisov commented Sep 16, 2023

kchibisov left a comment

kchibisov commented Sep 17, 2023

kchibisov Sep 17, 2023

DJMcNab Sep 17, 2023

elinorbgr Sep 18, 2023

ids1024 Sep 18, 2023 •

edited

Loading

kchibisov Sep 18, 2023

DJMcNab commented Sep 17, 2023

kchibisov commented Sep 17, 2023

elinorbgr commented Sep 19, 2023

kchibisov left a comment

kchibisov Sep 20, 2023

kchibisov Sep 20, 2023

kchibisov Sep 20, 2023

kchibisov Sep 20, 2023

DJMcNab Sep 20, 2023 •

edited

Loading

kchibisov Sep 20, 2023

		// Wayland-backend should probably document some guarantees here to make this sound,
		// but this is unlikely to change

		// In theory, we would need to call the process_events handler on fd. However,
		// we know that Generic's process events's call is a no-op, so we can just handle the event ourselves

		read_guard: Option<ReadEventsGuard>,
		fake_token: Option<Token>,

		// Otherwise, drop the guard if we have it, as we don't want to do any
		// reading when we didn't get any events

		if let Some(guard) = guard {
		if let Err(WaylandError::Io(err)) = guard.read() {

	#![forbid(unsafe_op_in_unsafe_fn)]
	#![deny(unsafe_op_in_unsafe_fn)]

Update dependencies and add an example #1

Update dependencies and add an example #1

Conversation

DJMcNab commented Sep 13, 2023

kchibisov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchibisov commented Sep 16, 2023

kchibisov left a comment

Choose a reason for hiding this comment

kchibisov commented Sep 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ids1024 Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJMcNab commented Sep 17, 2023

kchibisov commented Sep 17, 2023

elinorbgr commented Sep 19, 2023

kchibisov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJMcNab Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ids1024 Sep 18, 2023 •

edited

Loading

DJMcNab Sep 20, 2023 •

edited

Loading