Setup Forest execution threads #236
Conversation
austinabell
requested review from
ansermino,
dutterbutter and
ec2
as code owners
forest/src/main.rs
Outdated
|
|
||
| task::block_on(async move { | ||
| // Start libp2p service | ||
| let lp2p_service = Libp2pService::new(logger, &config.network, net_keypair); |
There was a problem hiding this comment.
I would just call it p2p_service
There was a problem hiding this comment.
unchanged in this PR, but changed it haha
forest/src/main.rs
Outdated
| if running.load(Ordering::SeqCst) > 0 { | ||
| // TODO change dropping threads to gracefully shutting down services | ||
| // or implement drop on components with sensitive shutdown | ||
| drop(lp2p_thread); |
|
@ansermino Changed to blocking on channel from ctrlc thread to avoid that busy loop, sorry to make change after review |
| ctrlc::set_handler(move || { | ||
| let prev = running.fetch_add(1, Ordering::SeqCst); | ||
| if prev == 0 { | ||
| println!("Got interrupt, shutting down..."); |
There was a problem hiding this comment.
nit: "Got interrupted, shutting down..."
There was a problem hiding this comment.
I think the noun version makes more sense in this context, anyone else have a preference? Idrc happy to change
There was a problem hiding this comment.
I'm okay with either. They're both clear enough. Maybe "Shutdown signal received... Shutting down" might be better? Again, indifferent though
There was a problem hiding this comment.
haha to avoid going down this rabbit hole I'm just going to pull in as is, I'll approve any change if anyone cares that much :)
Summary of changes
Changes introduced in this pull request:
Currently, the networking service was running in the main thread. This was always planned to run in seperate thread and this needs to change for the changes I'm working on. PRing in now because it can be isolated from the other changes I'm making but have testing running multiple threads and the SIGINT handler functions as intended.
To summarize the SIGINT handler, pressing once will drop all threads (and when services need to be carefully shut down that logic can be added) and a second press will force exit the main thread. With thread/task sleeps you can test this yourself.
Also the
process::exit()will not call destructors on any thread and will exit the process immediately, docs: https://doc.rust-lang.org/std/process/fn.exit.html and maybe should change the exit code to a non-zero one? Makes sense to me but I'm okay with changingReference issue to close (if applicable)
Closes
Other information and links