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

Improve global state management and concurrency (Context, Controller). #64

Merged
merged 1 commit into from
Feb 24, 2016

Conversation

mcav
Copy link
Contributor

@mcav mcav commented Feb 22, 2016

This commit improves and simplifies the way we manage global state, in collaboration with @jedireza.

With this commit:

  • Controller coordinates the interaction of services, servers, and events. ContextTrait has been merged into Controller; FoxBox is the concrete implementation of Controller.

    (Why the name change? In practice, Context was acting more like a controller than a context; as was Controller. The term Controller is more precise.)

  • We no longer coarsely lock the global state. Previously, we passed around Arc<Mutex<Context>> everywhere (aliased as SharedContext). When we needed to access the global context, we would coarsely lock() the mutex. This is inefficient and cumbersome. Instead, we only lock what we need to.

This allows us to clone and share references to a generic Controller (i.e. <T: Controller>) safely and concurrently.

NOTE: Git did not detect the rename of context.rs to controller.rs; the diff is slightly misleading in that regard. I'd recommend using the split view for easier reading.

@mcav mcav force-pushed the context-refactor branch 7 times, most recently from 7eb245a to 729530f Compare February 24, 2016 00:26
@mcav mcav changed the title [WIP] Decouple Context. Improve global state management and concurrency (Context, Controller). Feb 24, 2016
@mcav
Copy link
Contributor Author

mcav commented Feb 24, 2016

r? @fabricedesre

None => {
let mut response = Response::with(format!("No Such Service: {}", id));
response.status = Some(Status::BadRequest);
response.headers.set(AccessControlAllowOrigin::Any);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you forgot to add that one back when you rebased.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good catch.

@fabricedesre
Copy link
Collaborator

Very nice!
One nit: Context was meant to carry all the "shared" things. Now we end up exposing things like start_tunnel() and stop_tunnel() to places that should not have access to that. We should break that up in a follow-up.

Please fix the cors header and we'll merge.

@jedireza
Copy link
Contributor

🙌

mcav added a commit that referenced this pull request Feb 24, 2016
Improve global state management and concurrency (Context, Controller). r=fabrice
@mcav mcav merged commit 157a7f1 into fxbox:master Feb 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants