teloxide / dptree

Asynchronous event dispatch for Rust
https://docs.rs/dptree/latest/dptree/
MIT License
140 stars 8 forks source link

Issue while with trait objects and dptree. #27

Open felipet opened 3 months ago

felipet commented 3 months ago

Hi, I'm trying to pass a trait object as a dependency of the Dispatcher::builder and I can't figure out what I'm doing wrong with it because I get a runtime panick asking for the same type that I seem to be providing:

alloc::sync::Arc<alloc::boxed::Box<dyn finance_api::Market + core::marker::Send + core::marker::Sync>> was requested, but not provided. Available types:
    shortbot::State
    shortbot::CommandEng
    teloxide_core::types::me::Me
    teloxide_core::types::message::Message
    teloxide_core::types::update::Update
    teloxide::dispatching::dialogue::Dialogue<shortbot::State, teloxide::dispatching::dialogue::storage::in_mem_storage::InMemStorage<shortbot::State>>
    alloc::sync::Arc<alloc::boxed::Box<dyn finance_api::Market + core::marker::Send + core::marker::Sync>> <== THIS ONE!!
    alloc::sync::Arc<teloxide::dispatching::dialogue::storage::in_mem_storage::InMemStorage<shortbot::State>>
    teloxide_core::bot::Bot

note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
syrtcevvi commented 3 months ago

Seems strange. I think, for a fast workaround you should use just plain Arc, without inner Box

When I use trait objects in my bots, I wrap it into Arc, so just try to get rid of the inner Box:

Arc<dyn finance_api::Market + Send + Sync>
WaffleLapkin commented 3 months ago

Hm, my only idea so far is that you have multiple versions of finance_api in the tree. Try running cargo tree | grep finance_api or checking Cargo.lock file, to see if you are only using a single version of the crate.

Otherwise, could you provide an MRE (minimal reproducible example)?

Also as a note, you don't have to have Arc<Box<dyn ...>> you can have Arc<dyn ...> directly.

felipet commented 3 months ago

Seems strange. I think, for a fast workaround you should use just plain Arc, without inner Box

When I use trait objects in my bots, I wrap it into Arc, so just try to get rid of the inner Box:

Arc<dyn finance_api::Market + Send + Sync>

I made the change, but I get an alike error message:

thread 'tokio-runtime-worker' panicked at /home/felipe/.cargo/registry/src/index.crates.io-6f17d22bba15001f/dptree-0.3.0/src/di.rs:164:17:
alloc::sync::Arc<dyn finance_api::Market + core::marker::Send + core::marker::Sync> was requested, but not provided. Available types:
    alloc::sync::Arc<teloxide::dispatching::dialogue::storage::in_mem_storage::InMemStorage<shortbot::State>>
    teloxide_core::bot::Bot
    teloxide_core::types::message::Message
    teloxide_core::types::me::Me
    shortbot::CommandEng
    alloc::sync::Arc<dyn finance_api::Market + core::marker::Send + core::marker::Sync>
    teloxide_core::types::update::Update
    teloxide::dispatching::dialogue::Dialogue<shortbot::State, teloxide::dispatching::dialogue::storage::in_mem_storage::InMemStorage<shortbot::State>>
    shortbot::State

note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
felipet commented 3 months ago

Hm, my only idea so far is that you have multiple versions of finance_api in the tree. Try running cargo tree | grep finance_api or checking Cargo.lock file, to see if you are only using a single version of the crate.

Otherwise, could you provide an MRE (minimal reproducible example)?

Also as a note, you don't have to have Arc<Box<dyn ...>> you can have Arc<dyn ...> directly.

I think that I run the same version:

├── finance_api v0.1.0 (/home/felipe/Workspace/Rust/finance_api)
│   ├── finance_api v0.1.0

The implementation of the Market trait is in another library, and I checked that I only use a single version. I guess there's something odd with that implementation because I wrote a simpler example of the same idea and it works. I'll push the changes in case you can take a look. Anyway, thanks for the help!

felipet commented 3 months ago

Hi, I tried to rewrite a little bit of the code and I still get the same issue. I'm going to get rid of the trait stuff, and directly include the implementation that I need from my other library into the telegram bot. I need to move forward, and I'll get back to this later to properly use my external lib. Thanks for the help, whenever I have any update and all the code is pushed, I'll update this issue.

WaffleLapkin commented 3 months ago

@felipet can you provide a link to your code that would allow us to debug the issue?...

felipet commented 3 months ago

Sure @WaffleLapkin . Sorry for the delay, I wanted to tidy up the code so it would be easier to read. You can download the code for the bot from here: shortbot. In brief, the code it diverges from works. The main change I made respect to my previous code is that I copied the code from the lib finance_ibex and reimplemented locally the objects Ibex35Market and IbexCompany so I don't need to implement the traits Market and Company. The branch trait_error undoes such, so you can reproduce the error.

Then, you'll need a dirty step, which is to clone locally one dep: finance_ibex, and apply the attached patch to modify the code and make it async-compatible. I'm reluctant to push it because I made those changes quickly to see if it would work or not. As it didn't, I postpone doing it properly until I have more time.

To replicate the issue, just export a token for a telegram API using this variable:

export SHORTBOT__APPLICATION__API_TOKEN=<your token>

Then run the app. If you run the bot's command /choosestock the issue should come up.

async_changes.zip

WaffleLapkin commented 3 months ago

@felipet are you sure the code in the branch still produces the same error?..

The handler has stock_market: Arc<dyn Market + Sync + Send> while you add what looks like Arc<Box<dyn Market>> (load_ibex35_companies returns Box<dyn Market> and then you wrap it in an Arc).

WaffleLapkin commented 3 months ago

Also re: reproduce steps ... that is a bit much. Generally it is expected that the big reporter creates a MRE/minimization so that it's easy for other people to reproduce. Cloning another dep and patching it with a zip is more effort than I'm ready to do....

Btw your repo also does not contain a Cargo.lock, so it's not very reproducible.

felipet commented 3 months ago

@WaffleLapkin yeah sorry for the code, it's not actually generating exactly the same error right now. About the way to reproduce the error and so, as I said, I'll push the code once it's ready. I asked initially just to know if there was any limitation that you'd know about the use of traits in this particular scenario. Since you asked for the code even when I said that I'll post it once it's ready, I tried to push something for you.

I was not expecting you to fix anything, I was just asking in case there was any limitation about using trait objects along dependencies of the dispatcher. Given that you didn't mention it exists, please give me some time to rework that code once I have more time for it. Using a trait object is not my priority right now, so I took a particular implementation of the trait to continue with the other features that have a higher level of priority.

About the Cargo.lock, I prefer not to include it when I only use dependences from Crates.io as you can fully specify versions and features.

Thanks once again.

WaffleLapkin commented 3 months ago

@felipet no worries. your original error looked like a bug in teloxide (since it fossils that the type is not there and then mentions the same type), so I wanted to see if it really is a bug in teloxide.

And that wouldn't require you to change code, since you did get the error already, so you could literally take the code as-is and it would reproduce the error (and then maybe remove some things to minimize the example).

But either way, if you are not interested, then don't sweat :)

About the Cargo.lock, I prefer not to include it when I only use dependences from Crates.io as you can fully specify versions and features

That doesn't really work?...

In the sense that if a dependency is updated to a semver compatible version, cargo will use the new version, potentially changing things. Even if you use "=x.y.x" forcing cargo to use a specific version, your dependencies won't do the same, so the transitive dependencies will still be able to update. The only way to get anywhere near reproducible builds while using things from crates.io is to have a lock file.

felipet commented 3 months ago

Thanks @WaffleLapkin for the explanation regarding Cargo.lock. You're absolutely right, I've got biased by my previous job in which once a version was released, what you mentioned was more than prohibited. I was no longer considering it, but it's true that it can happen, so I'll push the lock always from now.

Regarding the old code, I was stuck with that issue for a few weeks, and when I got feed up with it, I simply wiped it and reimplemented the code without traits. The v0.1 doesn't support async code, and that's why those changes that provoked the issue are gone. The idea behind the trait is to allow more stock markets in the future, which is really very unlikely to happen. So I ended kicking out the trait and using straight a particular implementation. It's a nice to have thing, so as soon as I have time to prepare the code and properly describe the issue I'll do it.

felipet commented 2 months ago

Hi again @WaffleLapkin , I wrote a simple code example that reproduces the issue. Please, check the code out of here. The steps to reproduce the issue are listed in the README file.

I simplified the code example: I defined a struct which holds an u16 so Sync + Send are there for free. The struct implements a simple trait, and that's all. The handler expects a trait object that implements such trait + Send + Sync. That's it. Let me know if this code example fulfils your needs to easily reproduce the issue.

syrtcevvi commented 2 months ago

Hey. I've just tried your example out.

image

So, the problem is solved by:

    let data = Arc::new(SomeData::new()) as Arc<dyn DataTrait + Send + Sync>;

So, dynamic dispatching and types are tricky, because previously your data had type Arc<SomeData> but the requested one is actually Arc<dyn DataTrait + Send + Sync>

Just do the type conversion

felipet commented 2 months ago

Thanks @syrtcevvi for the solution. I didn't consider the casting because the compiler accepted the code.

WaffleLapkin commented 2 months ago

Note that this is a separate issue, since the error should have said that the context has Arc<SomeData> and not Arc<dyn DataTrait + Send + Sync>.