⬆️ zb: Swtich to new smol-rs crates #494

MaxVerevkin · 2023-10-14T13:29:42Z

closes #479.

zeenix

Looks good otherwise. You may want to refer to #479 in the description btw.

zbus/src/connection/mod.rs

zbus/src/object_server/mod.rs

MaxVerevkin · 2023-10-16T15:21:10Z

You may want to refer to #479 in the description btw.

There aren't any other outdated smol crates, right?

zeenix · 2023-10-16T15:26:10Z

There aren't any other outdated smol crates, right?

Maybe not at the moment but all crates are being updated. async-broadcast for example has been update already for event-listener 3 but not yet released. I should roll out a release soon..

zeenix · 2023-10-16T15:32:17Z

Maybe not at the moment but all crates are being updated.

E.g smol-rs/async-io#155

MaxVerevkin · 2023-10-16T16:28:30Z

I should roll out a release soon..

Didn't know you were involved in smol-rs :)

zeenix · 2023-10-16T17:10:18Z

I should roll out a release soon..

Didn't know you were involved in smol-rs :)

Not as much as I should be or want to be. :) async-broadcast 0.6.0 now out btw.

MaxVerevkin · 2023-10-16T18:49:05Z

I don't understand how CI successfully completed on everything except windows_test. It certainly doesn't work on my linux.

zeenix · 2023-10-16T21:18:42Z

Closes #479

As is, it does not closes #479 fully. The issue covers all smol-rs bumps that will be needed.

zeenix · 2023-10-16T21:20:00Z

I don't understand how CI successfully completed on everything except windows_test. It certainly doesn't work on my linux.

Gives you a clue at least that it's some timing issue? Also, better this than the other way around. Locally reproducible issues are much easier to debug/solve.

MaxVerevkin · 2023-10-17T11:26:10Z

Something's wrong with the tests, even on master... I remember they used to pass, but now fdpass_systemd consistently fails and unix_p2p inconsistently timeouts :(

---- tests::fdpass_systemd stdout ----
thread '<unnamed>' panicked at zbus/src/lib.rs:323:14:
called `Result::unwrap()` on an `Err` value: MethodError(OwnedErrorName("org.freedesktop.DBus.Error.InteractiveAuthorizationRequired"), Some("Interactive authentication required."), Msg { type: Error, sender: UniqueName(":1.1"), reply-serial: 38, body: Signature("s") })
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'tests::fdpass_systemd' panicked at zbus/src/lib.rs:311:5:
explicit panic

---- connection::tests::unix_p2p stdout ----
thread 'connection::tests::unix_p2p' panicked at zbus/src/connection/mod.rs:1437:5:
timeout: the function call took 15000 ms. Max time 15000 ms

(the output is from main branch)

zeenix · 2023-10-17T13:55:41Z

Something's wrong with the tests, even on master... I remember they used to pass, but now fdpass_systemd consistently fails and unix_p2p inconsistently timeouts :(

Ah yes, this one I've seen happening on my machine too. It disappears after a while though. I'd suggest skipping it as it shouldn't be affected by this PR.

zeenix · 2023-10-17T14:06:39Z

@MaxVerevkin seems like an event-listener regression on Windows:

thread 'zbus::Connection executor' panicked at C:\Users\runneradmin.cargo\registry\src\index.crates.io-6f17d22bba15001f\event-listener-3.0.0\src\lib.rs:1069:36:
listener was never inserted into the list

zeenix · 2023-10-17T20:32:56Z

@MaxVerevkin @notgull pointed out that the EventListener::new doesn't setup the listener to listen to the given event but rather must be told to do so using listen method. I think the new API is very unintuitive and hopefully we'll have some sort of resolution in event-listener for it but it gives you a clue of what must be wrong here.

MaxVerevkin · 2023-10-17T21:17:48Z

the EventListener::new doesn't setup the listener to listen to the given event but rather must be told to do so using listen method. I think the new API is very unintuitive and hopefully we'll have some sort of resolution in event-listener for it but it gives you a clue of what must be wrong here.

Even if .await is used? Apparently yes, that's unfortunate. I've finally found the problem, just had to run cargo test -- --nocapture to see the panic message from a different thread.

zeenix · 2023-10-17T21:32:50Z

Even if .await is used?

Yes because even though EventListener::new takes a reference to an Event, it can't already set itself to listen to it because that requires a pinned listener. It's bad, I know! BTW do join our matrix channels for discussions (zbus is currently at 99 members so you can set a record :)).

https://matrix.to/#/#zbus:matrix.org
https://matrix.to/#/#smol-rs:matrix.org

zeenix · 2023-10-17T21:35:33Z

Thanks for fixing! One last reservation though: If we only bump our direct event-listener dep, we'll now end up with 2 event-listener versions in our deps, the other through other smol-rs deps. Perhaps it'd make sense to resolve #479 completely in one PR?

MaxVerevkin · 2023-10-17T21:35:41Z

it can't already set itself to listen to it because that requires a pinned listener.

Can't it do so the first time it is polled?

zeenix · 2023-10-17T21:36:17Z

it can't already set itself to listen to it because that requires a pinned listener.

Can't it do so the first time it is polled?

Perhaps, please comment on smol-rs/event-listener#89

zeenix

Nice! Great to see you going the extra mile. 👍 Apart from the nit:

Just like in case of event-hander, async-broadcast now provides new API to avoid allocation when sending. We should either try to switch to that already here or leave a FIXME comment in the code and commit message about that.
We still have a bunch more crates I think: async-io (it was just released I believe) and async-process I think.

zbus/tests/e2e.rs

MaxVerevkin · 2023-10-18T13:07:18Z

Just like in case of event-hander, async-broadcast now provides new API to avoid allocation when sending

I found only one place where this can be done. Done. By the way, the changelog didn't mention this at all, so...

MaxVerevkin · 2023-10-18T13:08:35Z

I did not update async-io because it involves unsafe code, which I would like to avoid in this PR.

zeenix · 2023-10-19T10:34:12Z

I did not update async-io because it involves unsafe code, which I would like to avoid in this PR.

I don't think that's a good reason to split the whole PR (it's the exactly same topic/general change) but if you prefer it that way, sure. In that case, please do edit the description again to not get #479 closed with this merged.

zeenix · 2023-11-01T17:35:42Z

I did not update async-io because it involves unsafe code, which I would like to avoid in this PR.

Hm.. would this mean we'll be depending on multiple versions of event-listener? If so, I'd have to go back on my word and ask you to already port to async-io in this PR. Otherwise, we can merge already.

MaxVerevkin · 2023-11-10T08:28:48Z

zbus/Cargo.toml

@@ -108,7 +108,7 @@ nix = { version = "0.27", default-features = false, features = [
 [target.'cfg(target_os = "macos")'.dependencies]
 # FIXME: This should only be enabled if async-io feature is enabled but currently
 # Cargo doesn't provide a way to do that for only specific target OS: https://github.com/rust-lang/cargo/issues/1197.
-async-process = "1.7.0"
+async-process = "2.0.0"


Did not and can not test this change.

Me neither. :) I think some tests cover this so CI will tell us.

zeenix · 2023-11-23T15:20:56Z

@MaxVerevkin Hey, event-listener 4 was released that removes the footgun you encountered and its dependent smol-rs crates have been updated too. Do you think you'll be able to finish this PR soon with those updates?

MaxVerevkin · 2023-11-23T15:27:09Z

Hey, event-listener 4 was released that removes the footgun you encountered

Thanks for notifying, will look into it!

Do you think you'll be able to finish this PR soon with those updates?

I hope :) I'll have some time later today.

MaxVerevkin · 2023-11-23T19:18:19Z

@zeenix what do you think Connection::monitor_activity should do? Return Pin<Box<EventListener>>, Arc<Event> or &Event?

zeenix · 2023-11-23T22:47:39Z

@zeenix what do you think Connection::monitor_activity should do? Return Pin<Box<EventListener>>, Arc<Event> or &Event?

If our examples work with EventListener only then we return that and let the user pin it if they need to. Otherwise, we should return Pin<Box<EventListener>>.

notgull · 2023-11-23T22:52:03Z

Just a heads up, async-lock and async-channel don't use event-listener v4 yet. They still need a release. Usually I do a release run on the weekend.

MaxVerevkin · 2023-11-24T07:28:04Z

If our examples work with EventListener only then we return that

Return what exactly? Returning unpinned EventListener is pointless but returning pinned listener defeats the improvements of event-listener.

zeenix · 2023-11-24T12:07:11Z

If our examples work with EventListener only then we return that

Return what exactly? Returning unpinned EventListener is pointless

How so? You can .await it, can't you? If it was a useless type on its own, the new API of event-listener wouldn't make any sense.

but returning pinned listener defeats the improvements of event-listener.

Yeah, hence the preference to return EventListener.

zeenix · 2023-11-24T12:07:37Z

Just a heads up, async-lock and async-channel don't use event-listener v4 yet. They still need a release. Usually I do a release run on the weekend.

Thanks, I got ahead of myself. :)

MaxVerevkin · 2023-11-24T12:14:26Z

Return what exactly? Returning unpinned EventListener is pointless

How so?

pub fn monitor_activity(&self) -> EventListener {
    EventListener::new()
}

MaxVerevkin · 2023-11-24T12:20:59Z

Do you want me to update event-listener in a new commit or override the previous one?

zeenix · 2023-11-24T12:38:49Z

pub fn monitor_activity(&self) -> EventListener {
    EventListener::new()
}

Ah right, I forgot the exact semantics of the weird new API. :( Yes, I think we have to go with Pin<Box<EventListener>> then. I don't imagine many users to use more than one listener in their whole application so the allocation is minimal and not something to care about IMO.

Do you want me to update event-listener in a new commit or override the previous one?

No point in keeping the current one if we're going to change it in the same PR. :) So the latter.

MaxVerevkin · 2023-11-25T08:32:13Z

No point in keeping the current one if we're going to change it in the same PR. :) So the latter.

Is it ok to leave both? I don't want to mess up the entire git history 😆

MaxVerevkin · 2023-11-25T08:35:18Z

 thread 'tests::basic_connection_async' panicked at zbus/src/lib.rs:271:5:
explicit panic

Ntot sure whats going on in the CI...

zeenix · 2023-11-26T15:12:19Z

Is it ok to leave both? I don't want to mess up the entire git history 😆

You won't be messing up anything, only minimizing commits but sure, no biggie if you don't want to do it.

Ntot sure whats going on in the CI...

Well you have to debug obviously. There is some error from the low-level with async-io: https://github.com/dbus2/zbus/actions/runs/6988036119/job/19015207530?pr=494#step:6:705

MaxVerevkin · 2023-11-28T14:38:12Z

I don't know anything about low-level workings of zbus+async-io and honestly don't really want to dig in that direction. I think I'm not the right person to fix this :(

zeenix · 2023-11-28T14:54:13Z

I don't know anything about low-level workings of zbus+async-io

AFAICT the main errors were from the compiler so I'm sure there were much lowlevel stuff involved.

I think I'm not the right person to fix this :(

You came so close so it's sad to see you give up on what seems to be last hurdle w/o trying. Of course, it's your decision in the end. Just that I spent time reviewing too. Hopefully that helps me complete the work though.

MaxVerevkin · 2023-11-28T15:01:00Z

By the way to anyone who wants to continue from where I left off, feel free to reuse my commits, no attribution needed.

zeenix requested changes Oct 16, 2023

View reviewed changes

zbus/src/connection/mod.rs Show resolved Hide resolved

zbus/src/object_server/mod.rs Show resolved Hide resolved

MaxVerevkin force-pushed the event branch from 01b486d to 2d47c5b Compare October 16, 2023 17:15

MaxVerevkin force-pushed the event branch from 2d47c5b to 8498065 Compare October 17, 2023 21:17

MaxVerevkin force-pushed the event branch from 2200a39 to a4cb91f Compare October 17, 2023 21:54

zeenix changed the title ~~⬆️ zb: Update event-listener to 3.0.0~~ ⬆️ zb: Swtich to new smol-rs crates Oct 18, 2023

zeenix requested changes Oct 18, 2023

View reviewed changes

zbus/tests/e2e.rs Outdated Show resolved Hide resolved

MaxVerevkin force-pushed the event branch 3 times, most recently from f58cac3 to ba1a0c2 Compare October 18, 2023 13:04

MaxVerevkin added 6 commits November 10, 2023 10:17

⬆️ zb: update event-listener to 3.0

838c106

⬆️ zb: update async-broadcast to 0.6

fac1c34

⬆️ zb: update async-fs to 2.0

be62e89

⬆️ zb: update async-lock to 3.0

455c802

⬆️ zb: update async-io to 2.0

5e726aa

⬆️ zb: update async-process to 2.0

fe2ee2d

MaxVerevkin force-pushed the event branch from ba1a0c2 to fe2ee2d Compare November 10, 2023 08:27

MaxVerevkin commented Nov 10, 2023

View reviewed changes

⬆️ zb: update event-listener to v4.0.0

6010611

MaxVerevkin closed this Nov 28, 2023

TTWNO mentioned this pull request Nov 30, 2023

Actual smol rs update #517

Merged

MaxVerevkin deleted the event branch December 8, 2023 19:15

⬆️ zb: Swtich to new smol-rs crates #494

⬆️ zb: Swtich to new smol-rs crates #494

Conversation

MaxVerevkin commented Oct 14, 2023 • edited Loading

zeenix left a comment

Choose a reason for hiding this comment

MaxVerevkin commented Oct 16, 2023

zeenix commented Oct 16, 2023

zeenix commented Oct 16, 2023

MaxVerevkin commented Oct 16, 2023

zeenix commented Oct 16, 2023

MaxVerevkin commented Oct 16, 2023

zeenix commented Oct 16, 2023

zeenix commented Oct 16, 2023

MaxVerevkin commented Oct 17, 2023 • edited Loading

zeenix commented Oct 17, 2023

zeenix commented Oct 17, 2023 • edited Loading

zeenix commented Oct 17, 2023

MaxVerevkin commented Oct 17, 2023 • edited Loading

zeenix commented Oct 17, 2023

zeenix commented Oct 17, 2023

MaxVerevkin commented Oct 17, 2023

zeenix commented Oct 17, 2023

zeenix left a comment

Choose a reason for hiding this comment

MaxVerevkin commented Oct 18, 2023

MaxVerevkin commented Oct 18, 2023

zeenix commented Oct 19, 2023

zeenix commented Nov 1, 2023

MaxVerevkin Nov 10, 2023

Choose a reason for hiding this comment

zeenix Nov 10, 2023

Choose a reason for hiding this comment

zeenix commented Nov 23, 2023

MaxVerevkin commented Nov 23, 2023

MaxVerevkin commented Nov 23, 2023

zeenix commented Nov 23, 2023

notgull commented Nov 23, 2023

MaxVerevkin commented Nov 24, 2023

zeenix commented Nov 24, 2023

zeenix commented Nov 24, 2023

MaxVerevkin commented Nov 24, 2023

MaxVerevkin commented Nov 24, 2023

zeenix commented Nov 24, 2023

MaxVerevkin commented Nov 25, 2023

MaxVerevkin commented Nov 25, 2023

zeenix commented Nov 26, 2023

MaxVerevkin commented Nov 28, 2023

zeenix commented Nov 28, 2023

MaxVerevkin commented Nov 28, 2023

MaxVerevkin commented Oct 14, 2023 •

edited

Loading

MaxVerevkin commented Oct 17, 2023 •

edited

Loading

zeenix commented Oct 17, 2023 •

edited

Loading

MaxVerevkin commented Oct 17, 2023 •

edited

Loading