Suppress spurious x11_clipboard Error::Owner errors

[d-e-s-o]

The x11_clipboard::Clipboard::store() method has a racy ownership check that is causing spurious warnings to be emitted in alacritty [0]. x11_clipboard is unwilling to accept a fix for the issue [1], citing "maintenance mode".

Let's do the next best thing and ignore the error in our clipboard wrapper: for our intents and purposes this variant serves no purpose, because it only detects clipboard ownership changes after contents have already been stored, at which point the job could equally well be considered done.

[0] alacritty/alacritty#6978
[1] quininer/x11-clipboard#53

[nixpulvis]

If c11 clipboard is really in maintenance mode, maybe we should fork it?

[chrisduerr]

If c11 clipboard is really in maintenance mode, maybe we should fork it?

I'll have to think about this a little more and probably reach out upstream, but the attempts to fix this at the source by @d-e-s-o are commendable and seem very reasonable to me. Denying a patch like that without any further explanation suggests to me that x11-clipboard is not in maintenance mode, but it is unmaintained. Fixing bugs is maintenance.

For this issue specifically I'd be tempted to just use this patch since it's the easy way out, but that might be a bit too lazy.

[nixpulvis]

Out of curiosity, how should this work in a perfect world without upstream bugs? What does a working ownership check do, and what would ignoring it mean for Alacritty?

[d-e-s-o]

Out of curiosity, how should this work in a perfect world without upstream bugs? What does a working ownership check do, and what would ignoring it mean for Alacritty?

Without upstream bugs we don't need the patch and we wouldn't get spurious errors as we see them. I don't think the ownership check has any relevance for Alacritty. I am also not sure what the reasoning for having it upstream is, to be honest. I traced it back to some ancient commit upstream that of course doesn't have any description other than "[Added] start impl store".

As per my understanding, I think it can make sense to verify that the "store" was successful if the time stamp provided is in the past -- in which case the operation could have failed, because another client raced ahead of you and the server now considered the operation out of date and discarded it. However, here the setting happens with CURRENT_TIME, so I don't believe that to be possible. Instead, what now can happen is that shortly after we "stored" successfully, another client takes ownership of the clipboard and now the subsequent check fails. But it really doesn't tell you as the issuer anything. It's entirely legitimate for that to happen: there is not and cannot be an assumption of exclusivity of a shared resource (the clipboard).

[nixpulvis]

Yea looking over the code a little bit, I'm confused why the ownership check is there at all too. If set_selection_owner fails it should return an Err result itself. Checking that a subsequent call to get_selection_owner seems completely unrelated as you pointed out due to concurrent access to the clipboard.

If we do simply ignore the result instead of forking, I suggest we add a note about this being dubious logic in the upstream code in case we ever want to revisit the decision later. Perhaps just link to the upstream PR.

[chrisduerr]

Just to follow up on this: It's definitely still in my backlog. Unfortunately I'm still waiting for clarification from upstream. I definitely want this included in the next Alacritty release (after 0.17.0), so if they don't get back to me, I might just merge your x11-clipboard PR and ask for forgiveness rather than permission.