citadel-tech / coinswap

Functioning, minimal-viable binaries and libraries to perform a trustless, p2p Maxwell-Belcher Coinswap Protocol
https://gist.github.com/chris-belcher/9144bd57a91c194e332fb5ca371d0964
Other
73 stars 45 forks source link

Join the recovery threads. #284

Open mojoX911 opened 1 month ago

mojoX911 commented 1 month ago

Problem

In the Maker server, we sometimes launch the recovery thread. But we don't join them after completion. They should be joined.

Ex: https://github.com/citadel-tech/coinswap/blob/bab961479c780ea7ee8402772e83b942e5c443d0/src/maker/api.rs#L478-L483

Approach.

Shourya742 commented 1 month ago

Hey @mojoX911, I was thinking about the issue with the unjoined recovery threads in the Maker server. Instead of manually joining them, we could use scoped threads to handle this more cleanly. Using scoped threads would ensure that any threads we spawn would be guaranteed to complete within the scope they're created, and we wouldn’t need to manually track or join them at shutdown. What do you thoughts on this?

mojoX911 commented 1 month ago

If it works and keeps the behavior intact I am fine with it.

My only concern is, as the recovery thread takes a very long time to return (144 blocks of waiting), it will surely last longer than the calling function. I don't know if in case of scope threads it will make the calling function blocking on the spawned thread? If it does then it would hang the rest of the app until recovery finishes.

Whereas currently we have an async type behavior, where recovery can keep happening in the background, while all other app APIs remain operational.

KnowWhoami commented 1 month ago

I don't know if in case of scope threads it will make the calling function blocking on the spawned thread?

Yes, the scoped threads will block the calling function -> so IMO, we should not them in case, when we are spawning normal threads which are meant to do their task asynchronously. But there are several instance where we are waiting just after spawning a normal thread -> so in that case we can use these scoped ones.

mojoX911 commented 1 month ago

But there are several instance where we are waiting just after spawning a normal thread

Seems like all the threads in start_maker_server can be scoped? @Shourya742 ?

KnowWhoami commented 1 month ago

Seems like all the threads in start_maker_server can be scoped?

No It can't be because:

But there are several instance where we are waiting just after spawning a normal thread -> so in that case we can use these scoped ones.

Upon further reflection, I concluded that the choice between normal threads and scoped threads depends on the number of threads being used:

mojoX911 commented 2 weeks ago

I would say for now, we handle the joining of the recovery thread the basic way. Add it to a global threadpool which always joins at main server scope. Solves the problem at hand.

I am turning this into 1.0 as getting it done is not that hard. And without this, we can currently have dangling recovery threads, or we won't see panics from the recovery. Thats pretty bad.

later on, we can explore scoped threads more and track it via another issue. Redoing the server architecture better is in scope for 1.1.