Add threadpool to join the recovery threads

[claddyy]

Fixes #284

[Shourya742]

Any specific reason for adding another crate for thread handling?

[codecov]

Codecov Report

Attention: Patch coverage is 79.16667% with 10 lines in your changes missing coverage. Please review.

Project coverage is 78.18%. Comparing base (4747c6f) to head (014af60).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
src/maker/server.rs	68.18%	7 Missing ⚠️
src/maker/api.rs	86.95%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #316      +/-   ##
==========================================
- Coverage   78.26%   78.18%   -0.08%     
==========================================
  Files          32       32              
  Lines        4840     4859      +19     
==========================================
+ Hits         3788     3799      +11     
- Misses       1052     1060       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[claddyy]

Any specific reason for adding another crate for thread handling?

I tried implementing the methods from threadpool crate earlier, which was not needed. I've refined the implementation now.

[Shourya742]

The approach looks fine to me. However, since you are now adding the threadPool field to the Maker struct, ensure that this field is used consistently for all instances of the thread pool within Maker. In the Maker server, there is a local thread pool—please ensure you use this thread pool there as well. Additionally, you missed joining a thread in the Maker handler. Address these issues, and this PR will be good to go.

[KnowWhoami]

PartialACK.
Here are the list of review comments focusing on handling the threads and related errors in much better way.

[KnowWhoami]

@Shourya742

• we can't push the tor_handle inside the maker's thread_pool because tor_handle is of type mitosis::JoinHandle while thread_pool must contain JoinHandle of type std::thread::Joinhandle.
  Though right now , this is not an issue but may become a potential bug when implementing drop trait for Maker struct.

[KnowWhoami]

Also there are many instances in codebase where we are not handling the thread's error correctly -> so we incorporate those enhancements in this pr itself?
For eg:

coinswap/tests/test_framework/mod.rs

Lines 146 to 149 in 6e5ce00

	let directory_server_instance_clone = directory_server_instance.clone();
	thread::spawn(move || {
	start_directory_server(directory_server_instance_clone).unwrap();
	});

Here unwraping the start_directory_server inside the thread doesn't make sense -> as we fail to start directory server but still we continue with further procedure like initing a maker & taker etc and that's not good.
Instead , we must propagate the error to main thread and unwrap there -> this will panic the test at that stage itself which will be good UX for tests.

[claddyy]

I appreciate the efforts put to review this PR by @KnowWhoami, but I disagree with the most of the review. I thought of addressing the review in dev-sync, so we all can come to agreement, regarding what exactly is required.

The review suggests complete structural changes without clear technical justification. Comments with suggestion to remove ThreadPool struct, removing all the Traits including Drop trait(so that it can be implemented later; but imo this doesn't make difference), manually joining threads doesn't add any improvements.

The suggested changes include harder to maintain alternatives, without any concrete benefits.

PS: I'm always open to constructive feedback and improvements.

[Shourya742]

While I appreciate the time put to review this PR by @KnowWhoami, I disagree with the most of the review. I thought of addressing the review in dev-sync, so we all can come to consensus, regarding what exactly is required.

The review suggests complete structural changes without clear technical justification. Comments with suggestion to remove ThreadPool struct, removing all the Traits including Drop trait(so that it can be implemented later; but imo this doesn't make difference), manually joining threads appears to be based on personal preferences rather than improvements.

The suggested changes include harder to maintain alternatives, without any concrete benefits.

PS: I'm always open to constructive feedback and improvements.

@claddyy just add inline to join_all and the PR is good to go.

[Shourya742]

ACK

[mojoX911]

I like the structure. Ack.

Below is one non-blocking comment. Can be done in a followup PR too, but will be good to have to help for debugging later.

[mojoX911]

Also there are many instances in codebase where we are not handling the thread's error correctly -> so we incorporate those enhancements in this pr itself? For eg:

coinswap/tests/test_framework/mod.rs

Lines 146 to 149 in 6e5ce00

	let directory_server_instance_clone = directory_server_instance.clone();
	thread::spawn(move || {
	start_directory_server(directory_server_instance_clone).unwrap();
	});

Here unwraping the start_directory_server inside the thread doesn't make sense -> as we fail to start directory server but still we continue with further procedure like initing a maker & taker etc and that's not good. Instead , we must propagate the error to main thread and unwrap there -> this will panic the test at that stage itself which will be good UX for tests.

Not needed for the test framework. The only other place where it makes sense is the DNS server when it creates the rpc thread. But not a major need as DNS is not user facing.

[mojoX911]

Merging this to unblock downstream works.

[claddyy]

I like the structure. Ack.

Below is one non-blocking comment. Can be done in a followup PR too, but will be good to have to help for debugging later.

Addressed the review here #324.