Stores don't implement Send

[EcoloSweet]

Stores traits and implementations (for instance SessionStore and it's InMemSessionStore) don't implement the Send trait because of the #[async_trait(?Send)] macro.

I understand that maybe, this makes this trait is easier to implement but it also prevents further implementations from spawning tasks in different threads.

Crates like sled (used by presage for stores) do have the Send trait so it would be great if we could remove this limitation.

Is there another reason why we prevent from implementing this trait?

If you think this is not an error an this is useful, I can work on rewriting this part of code and submit a pull request.

[rubdos]

I think you'd be interested in this: signalapp/libsignal#298

We don't design those traits in this crate. There's more discussion on this example patch: signalapp/libsignal#297.

[EcoloSweet]

Ok thank you for this.

I don't think it will be implemented soon and I'm not good enough to create my own fork ... so let's wait 🤷

[rubdos]

I'll notify @gferon, he's probably more interested in it than I am (I'm running everything on a LocalSet anyway).

[gferon]

So, this is an excellent question and I'd love to move forward with this. The main problem is that libsignal-client itself doesn't require store traits to be Send - this is because it accepts a ctx raw pointer that's used in different platforms (e.g. a pointer to a database context or transaction context). I wish I'd had more time to experiment on the matter, I'll try to see if I can resume my efforts in the next few weeks.

[EcoloSweet]

Hey @gferon or @rubdos

Do you know if there is anything to do about this issue in libsignal? I don't feel like I have the shoulders to modify the code from signal, but I think it would be great to have a send manager...

Is there is a reason why nobody seems to be bothered by this issue, neither people using presage nor people using libsignal... Am I missing something? :)

[gferon]

Hey @EcoloSweet thanks for pinging me back. I must admit I haven't looked at this issue in a while, but I also feel like it's not really a big issue since all apps using this stack of libraries end-up using a single-threaded async runtime without any particular problem (@rubdos is this maybe a problem in whisperfish and I just don't know about it?).

I also don't really expect a huge benefit from being able to run presage or libsignal-service-rs in a multi-threaded async runtime, except avoiding the bad surprise when you get started and can't compile your basic demo. 😄

Obviously, I might be wrong and I'd love to know if you have some ideas why this can be worth the effort.

[EcoloSweet]

Thank you for replying so fast! 😄

I feel the same as you about the benefit that a multi-threaded implementation could bring to Signal. This would be almost useless.

However, in my current use of this crate, I have a program doing some heavy computing, program which is async and use multiple threads. When it needs to ping me, it is done through signal.

I would like my code to be like SignalPing::send_message(...) (with SignalPing struct containing manager) but since manager isn't send, i need to spawn a local task, with signal stuff living in a separate task and use channels to communicate with this task.

It works but it isn't very elegant.

However, I can see that there is a lot of effort to put in refactoring the code in multiple crates. I imagine this problem isn't that bad and doesn't worth spending hours on changing everything...

[rubdos]

However, I can see that there is a lot of effort to put in refactoring the code in multiple crates. I imagine this problem isn't that bad and doesn't worth spending hours on changing everything...

It's not only crates, it's also Android and iOS and NodeJS code. That's the most annoying part: making sure those all behave.

It's not like we don't want it to be Send. I've been looking at having Signal process the decryption in a separate thread, too. For Whisperfish specifically, I'm relying on Tokio's current_thread scheduler I/O behaviour for qmeta_async to work. So as long as I don't refactor that into the multi thread scheduler (and I don't know how, currently!), a Send implementation will even be useless for me :'-)

[EcoloSweet]

I don't see how this does not bother you, there must be something wrong in my implementation...

Since manager isn't send, it must be in a task from a localset. But if you are in an environment where you don't manage the runtime (for example in a library), how come you to be sure of the task executing in a localset?

I have to spawn a new thread, where I start a new runtime, where I create a local set task where I put my signal future. This doesn't seem right 🤣

[rubdos]

I actually do manage my own runtime, there's quite a thing going on in qmeta_async for that, including a manually managed LocalSet (for completely different reasons than what you state here). My whole application runs on a current_thread scheduler. The Presage examples also all run on a current_thread scheduler.

That said, I don't think you need to manage your own LocalSet? Can't you start a task that constructs your Signal client actor and spawns it with spawn_local?

[jonnius]

Just to give a little example were we stumbled upon this: crayfish

Background

Crayfish is a backend for Axolotl that was started to replace the old textsecure backend step by step with an libsignal-rs based approach. As the existing frontend and backends are web and go based, we chose to use a web socket to interface to crayfish.

Issue

We use warp to serve the local web socket and listen for requests. warp require a Send future as request handler.

Solution

We decoupled the web socket from the request processing via another server style mpsc tokio channel with oneshot callbacks (code). Alternatively, we could just replace warp with something single-threaded.