[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:
Related
related to SERVER-20441 ReplIndexPrefetch Server Parameter is... Closed
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:

ExportedServerParameter<int> MaxConsecutiveFailedChecksSetting(                                             
     ServerParameterSet::getGlobal(),                                                                        
     "replMonitorMaxFailedChecks",
     &ReplicaSetMonitor::maxConsecutiveFailedChecks,                                                         
     false,  // allowedToChangeAtStartup                                                                     
     true);  // allowedToChangeAtRuntime

We could either:
1) Prevent ExportedServerParameter to be configured with allowedToChangeAtRuntime = true, and make implementers having to inherit from ServerParameter directly, or

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: SERVER-20096: ExportedServerParameter<T> is not thread-safe for parameters changeable at runtime.
Branch: artree
https://github.com/10gen/mongo-enterprise-modules/commit/ae6f4c4f7a3cf5b992aa3972b7c6071c08746a81

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: SERVER-20096: ExportedServerParameter<T> is not thread-safe for parameters changeable at runtime.
Branch: master
https://github.com/10gen/mongo-enterprise-modules/commit/ae6f4c4f7a3cf5b992aa3972b7c6071c08746a81

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: SERVER-20096: ExportedServerParameter<T> is not thread-safe for parameters changeable at runtime.
Branch: master
https://github.com/mongodb/mongo/commit/1f6d501989875dac4bcd7cffbbabf320c3cab289

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.

Generated at Thu Feb 08 03:53:08 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.