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

Add mutex to protect node.staticChild #42

Closed
wants to merge 1 commit into from

Conversation

bastiankoetsier
Copy link
Contributor

This PR adds another mutex to protect staticChild for concurrent access.
Additionally, I cleaned up an unnecessary else branch

@dimfeld
Copy link
Owner

dimfeld commented Dec 28, 2016

I'm reluctant to put a mutex in the read path, as the vast majority of users aren't adding routes while the router is in use.

Let's use an RWMutex instead, and have just one over the entire tree, probably stored in the router, instead of one per node. And have an option in the router to explicitly enable use of this mutex in the read path. This is so that the atomic operations inside the RWMutex don't slow things down under high load if the user doesn't need the mutex behavior.

@bastiankoetsier
Copy link
Contributor Author

I understand your point. Maybe when I find the time I can come up with an according solution. Thanks for considering anyway!

@dimfeld
Copy link
Owner

dimfeld commented Dec 29, 2016

If I find the time I'll be glad to implement this myself :) It may take a few weeks before I'm able to get to it though.

@dimfeld
Copy link
Owner

dimfeld commented Dec 31, 2016

@bastiankoetsier I had time today to do this! Check out #43 and the latest release 3.7.0 to get the new feature.

@bastiankoetsier
Copy link
Contributor Author

@dimfeld nice! I really appreciate the work you put into this! Thank you very much! Keep on working hard and have a very happy new year!

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.

None yet

2 participants