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.

Sign up for GitHub

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

Jump to bottom

use separate thread for trapping signals #7

Merged
merged 1 commit into from
Oct 9, 2015
Merged

use separate thread for trapping signals #7

merged 1 commit into from
Oct 9, 2015

Conversation

reset
Copy link
Collaborator

@reset reset commented Oct 9, 2015

The purpose of this PR is two-fold:

  1. Practice and demonstrate a working knowledge of effective concurrency in Rust
  2. Clean up the main loop and make it event driven. This will allow additional worker threads, like the upcoming package updater, to send messages back to the main loop where they will be processed in serial. This will be useful for ensuring we enact on whatever message we received last.
    • Shut down? Sure.
    • Update the software? Ok, we'll do that next.
    • Re-configure the software? Up next!

This will be used to complete #5
/cc @adamhjk

@chef-delivery
Copy link
Contributor

This PR has passed 'Verify' and is ready for review and approval!
Use: '@delivery approve' when code review is complete.

1 similar comment
@chef-delivery
Copy link
Contributor

This PR has passed 'Verify' and is ready for review and approval!
Use: '@delivery approve' when code review is complete.

reset
reset reviewed Oct 9, 2015
View reviewed changes
src/bldr/util/signal_handler.rs
WHICH_SIGNAL.store(sig as usize, Ordering::SeqCst);
}

pub fn start() -> Handler {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the appropriate approach here is to pass in a sender/receiver from the caller instead of creating the sender/receiver here and returning the caller the sender to our receiver.

I'm wrestling with the borrower checker and lifetimes right now in hopes of having a better understanding. I might update this PR before merging it so we have this behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you did this right. No reason for the caller to have to worry about the channels - this way it's all hidden under the hood. Feels right to me.

@adamhjk
Copy link
Contributor

adamhjk commented Oct 9, 2015

I'm 👍 on this.

One thing I'm unsure of in my own fledgling rust-dom is where the idiomatic place to put a function is. We define a Handler struct, which makes sense - and then we hang most of the functions off the namespace. This was clearly the model that made sense to me, but I wonder if it's how most of Rust handles things (I look at the amount of stuff hanging off of Structs as opposed to the module namespaces in the standard library, for example).

This code looks good to me, though. I would ship it, and we can noodle about style later.

Ship it!

@chef-delivery
Copy link
Contributor

This PR has passed 'Verify' and is ready for review and approval!
Use: '@delivery approve' when code review is complete.

3 similar comments
@chef-delivery
Copy link
Contributor

This PR has passed 'Verify' and is ready for review and approval!
Use: '@delivery approve' when code review is complete.

@chef-delivery
Copy link
Contributor

This PR has passed 'Verify' and is ready for review and approval!
Use: '@delivery approve' when code review is complete.

@chef-delivery
Copy link
Contributor

This PR has passed 'Verify' and is ready for review and approval!
Use: '@delivery approve' when code review is complete.

@reset
Copy link
Collaborator Author

reset commented Oct 9, 2015

@adamhjk ready for final review and merge

@adamhjk
Copy link
Contributor

adamhjk commented Oct 9, 2015

One more thing - please squash your four commits to one, then I'll approve.

@reset
use separate thread for trapping signals
3a5cfb7 
coordinate with main loop over channels
@chef-delivery
Copy link
Contributor

This PR has passed 'Verify' and is ready for review and approval!
Use: '@delivery approve' when code review is complete.

reset added a commit that referenced this pull request Oct 9, 2015
@reset
Merge pull request #7 from chef/signal-channels
cde4a56 
use separate thread for trapping signals
@reset reset merged commit cde4a56 into master Oct 9, 2015
@reset reset deleted the signal-channels branch October 9, 2015 21:03
@adamhjk
Copy link
Contributor

adamhjk commented Oct 9, 2015

@delivery approve

@chef-delivery
Copy link
Contributor

Failed to approve change: 73e2b09b-1d3a-4c8d-9cda-fd9815f1c92c @adamhjk

@adamhjk
Copy link
Contributor

adamhjk commented Oct 9, 2015

@delivery deliver

@chef-delivery
Copy link
Contributor

Failed to deliver change: 73e2b09b-1d3a-4c8d-9cda-fd9815f1c92c
Change must pass acceptance before delivering. @adamhjk

@adamhjk
Copy link
Contributor

adamhjk commented Oct 9, 2015

@delivery approve

@chef-delivery
Copy link
Contributor

Failed to approve change: 73e2b09b-1d3a-4c8d-9cda-fd9815f1c92c @adamhjk

@reset reset restored the signal-channels branch October 9, 2015 21:11
@reset
Copy link
Collaborator Author

reset commented Oct 9, 2015

@delivery approve

@chef-delivery
Copy link
Contributor

Failed to approve change: 73e2b09b-1d3a-4c8d-9cda-fd9815f1c92c @reset

@jtimberman jtimberman deleted the signal-channels branch February 25, 2016 02:47
jsirex pushed a commit to jsirex/biome that referenced this pull request Aug 22, 2019
@predominant
Merge pull request habitat-sh#7 from biome-sh/forkman-raw
21722f2 
Restore builkite files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@reset @chef-delivery @adamhjk