[SERVER-20096] ExportedServerParameter<T> is not thread-safe for parameters changeable at runtime. Created: 24/Aug/15 Updated: 13/Oct/15 Resolved: 22/Sep/15 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Internal Code |
| Affects Version/s: | None |
| Fix Version/s: | 3.1.9 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Andreas Nilsson | Assignee: | Mark Benvenuto |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||
| Operating System: | ALL | ||||||||
| Sprint: | Platform 9 (09/18/15), Platform A (10/09/15) | ||||||||
| Participants: | |||||||||
| Description |
|
ExportedServerParameter does not protect the underlying templated variable from concurrent access. The following construct is hence not safe if maxConsecutiveFailedChecks is ever read without holding a lock preventing a setParameter call:
We could either: 2) Use a mutex in ExportedServerParameter or use atomics for the template specializations where that make sense. Regardless we should audit the existing uses of ExportedServerParameter. |
| Comments |
| Comment by Githook User [ 13/Oct/15 ] |
|
Author: {u'username': u'markbenvenuto', u'name': u'Mark Benvenuto', u'email': u'mark.benvenuto@mongodb.com'}Message: |
| Comment by Githook User [ 22/Sep/15 ] |
|
Author: {u'username': u'markbenvenuto', u'name': u'Mark Benvenuto', u'email': u'mark.benvenuto@mongodb.com'}Message: |
| Comment by Githook User [ 22/Sep/15 ] |
|
Author: {u'username': u'markbenvenuto', u'name': u'Mark Benvenuto', u'email': u'mark.benvenuto@mongodb.com'}Message: |
| Comment by Mark Benvenuto [ 14/Sep/15 ] |
|
There are more cases of concurrency problems in Server Parameters then just ExportedServerParameter. If we want to fix all cases of this, then we need to fix all the classes derived from ServerParameter. Any class derived from ServerParameter is marked as set by runtime by default. |
| Comment by Andy Schwerin [ 31/Aug/15 ] |
|
On reflection, I think I'd like a variant of option 2. In this variant, the "settable at runtime" constructor flag is only available to specializations of ESP<T> where <T> is AtomicWord<U> for some U. Then, we take the existing ESP<T> instances that are marked "settable at runtime", and switch them to use AtomicWord and appropriately use loadRelaxed. We offer no runtime settable options for anything other than AtomicWord<T>. I like this option because it ought to "just work" anywhere that the existing runtime setting implementation has coincidentally been working today. |
| Comment by Andrew Morrow (Inactive) [ 25/Aug/15 ] |
|
schwerin - Am I correct in reading that you then are recommending option 1, and that we should require runtime mutable parameters to manage their own synchronization? |
| Comment by Andy Schwerin [ 24/Aug/15 ] |
|
I believe this is as-designed. I think option 2 with the mutex or atomic may be unworkable in practice for uses that are susceptible to the problem. I recommend improving the documentation. |