Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

switch from threads and channels to an actor model for concurrency (and fix horrible code) #5

Open
cdepillabout opened this issue Jul 17, 2020 · 0 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@cdepillabout
Copy link
Owner

Some of the break-time code is really horrible. There are a mess of thread-based state machines, all communicating over mpsc channels.

Here are the main state machines:

  • pub fn run(
    config: &Config,
    restart_wait_time_sender: Sender<InnerMsg>,
    ) -> ! {
    let idle_detector = Self::new(restart_wait_time_sender);
    let idle_detection_milliseconds =
    config.settings.idle_detection_seconds * 1000;
    loop {
    let time_before_sleep = SystemTime::now();
    let sleep_time_duration = Duration::from_secs(SLEEP_SECONDS);
    std::thread::sleep(sleep_time_duration);
    // Calculate the actual amount of time that has passed during sleep.
    // This will potentially be different from the sleep time because the computer could be
    // suspended during the above std::thread::sleep().
    let time_difference_milliseconds: u128 = time_before_sleep
    .elapsed()
    .unwrap_or(sleep_time_duration)
    .as_millis();
    // We subtract out the sleep time to get just the amount that the computer would have
    // been suspended for. If the computer wasn't actually suspended, then this should be
    // very close to 0.
    let suspend_milliseconds: u128 =
    time_difference_milliseconds - SLEEP_MILLISECONDS;
    let idle_query_res = xcb::screensaver::query_info(
    &idle_detector.conn,
    idle_detector.root_window,
    )
    .get_reply()
    .unwrap();
    let ms_since_user_input = idle_query_res.ms_since_user_input();
    // println!(
    // "ms_since_user_input: {}, suspend_milliseconds: {}",
    // ms_since_user_input,
    // suspend_milliseconds,
    // );
    if has_been_idle(
    idle_detection_milliseconds.into(),
    ms_since_user_input.into(),
    suspend_milliseconds,
    ) {
    idle_detector
    .restart_wait_time_sender
    .send(InnerMsg::HasBeenIdle);
    }
    }
    }
    }
  • break-time/src/scheduler.rs

    Lines 141 to 254 in 6bad134

    fn run_loop(&mut self) -> ! {
    loop {
    match self.state {
    State::CountDownToBreak => {
    let wait_until_break_result = self.wait_until_break();
    match wait_until_break_result {
    WaitUntilBreakResult::FinishedWaiting => {
    self.state = State::WaitingForBreakEnd;
    }
    WaitUntilBreakResult::Paused => {
    self.state = State::Paused;
    }
    }
    }
    State::Paused | State::WaitingForBreakEnd => {
    // Wait for a message signalling a break ending or a pause ending.
    println!("Scheduler currently waiting for a message signaling either a break or a pause ending.");
    let msg = self
    .break_ending_receiver
    .recv()
    .expect("Error receiving value in Scheduler.");
    match msg {
    Msg::Start => {
    self.state = State::CountDownToBreak;
    }
    }
    }
    }
    }
    }
    fn wait_until_break(&self) -> WaitUntilBreakResult {
    loop {
    let waiting_result = self.send_msgs_while_waiting();
    match waiting_result {
    WaitingResult::Finished => {
    println!(
    "Scheduler successfully finished sleeping, checking if it can break now..."
    );
    let (opt_can_break, errs) = self.plugins.can_break_now();
    if errs.is_empty() {
    match opt_can_break {
    None => panic!("If there are no errors, then we should always get a response to can_break"),
    Some(can_break) => {
    if can_break.into_bool() {
    println!("Scheduler realized it was able to break, so sending a message.");
    self.sender.send(super::Msg::StartBreak);
    return WaitUntilBreakResult::FinishedWaiting;
    } else {
    println!("Could not break right now, so sleeping again...");
    }
    }
    }
    } else {
    println!(
    "There have been some errors from our plugins:"
    );
    for e in errs {
    println!("{}", e);
    }
    println!("Sleeping again just to be safe...");
    }
    }
    WaitingResult::NeedToRestart => {
    // Just let this loop restart.
    println!(
    "Scheduler got a message to restart sleeping again, probably because X has been idle..."
    );
    }
    WaitingResult::Paused => {
    return WaitUntilBreakResult::Paused;
    }
    }
    }
    }
    fn send_msgs_while_waiting(&self) -> WaitingResult {
    self.sender.send(super::Msg::ResetSysTrayIcon);
    let mut remaining_time = self.time_until_break;
    for period in create_periods_to_send_time_left_message(self.time_until_break) {
    let opt_time_to_sleep = remaining_time.checked_sub(period);
    println!("In send_msgs_while_waiting loop for period {:?}, remaining_time: {:?}, time_to_sleep: {:?}", period, remaining_time, opt_time_to_sleep);
    match opt_time_to_sleep {
    None => {
    // This happens when the periods to send the time-left message are greater than
    // the remaining time. We can just skip this.
    }
    Some(time_to_sleep) => {
    let res = self
    .restart_wait_time_receiver
    .recv_timeout(time_to_sleep);
    match res {
    Ok(InnerMsg::HasBeenIdle) => {
    println!("HERERERERE");
    return WaitingResult::NeedToRestart;
    }
    Ok(InnerMsg::Pause) => {
    println!("Got inner message to pause...");
    return WaitingResult::Paused;
    }
    Err(_) => {
    self.sender.send(
    super::Msg::TimeRemainingBeforeBreak(period),
    );
    remaining_time -= time_to_sleep;
    }
    }
    }
    }
    }
    return WaitingResult::Finished;
    }
  • break-time/src/lib.rs

    Lines 31 to 67 in 6bad134

    fn handle_msg_recv(
    config: &Config,
    sender: glib::Sender<Msg>,
    scheduler_outer_sender: Sender<scheduler::Msg>,
    scheduler_inner_sender: Sender<scheduler::InnerMsg>,
    tray: &mut Tray,
    msg: Msg,
    ) {
    match msg {
    Msg::EndBreak => {
    println!("break ended");
    scheduler_outer_sender.send(scheduler::Msg::Start);
    }
    Msg::Pause => {
    tray.pause();
    scheduler_inner_sender.send(scheduler::InnerMsg::Pause);
    }
    Msg::Quit => {
    gtk::main_quit();
    }
    Msg::StartBreak => {
    println!("starting break");
    tray.render_break_starting();
    ui::start_break(config, sender);
    }
    Msg::ResetSysTrayIcon => {
    tray.render_normal_icon();
    }
    Msg::Resume => {
    tray.resume();
    scheduler_outer_sender.send(scheduler::Msg::Start);
    }
    Msg::TimeRemainingBeforeBreak(remaining_time) => {
    tray.update_time_remaining(remaining_time);
    }
    }
    }

It would be great to refactor this code to not be so horrible.

One of the main problems I had is that there are some types that can't easily be passed between threads, including handles for connecting to Google Calendar, and connection handles for X. Ideally, the code that handles both of these tasks would have their own threads.

I'm not sure the best solution to all these problems, but I was thinking that instead of using threads and channels for concurrency and communication, it would be easier to use some sort of service-oriented actor model, where each state machine would be it's own actor, as well as the parts of the code that can't be passed between threads.

Testing and simplifying all of the state-transitions would also be very helpful. There are probably some state transitions that don't make any sense, yet are still possible in the current code.

@cdepillabout cdepillabout added enhancement New feature or request help wanted Extra attention is needed labels Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

1 participant