SERVER-2942 Windows remapPrivateView() should unmap & remap

Change remapPrivateView() for Windows to use UnMapViewOfFile()
and MapViewOfFileEx() along with a mutex to reset the memory
map of the private view.

3 files changed, 71 insertions, 29 deletions

diff --git a/src/mongo/util/concurrency/remap_lock.h b/src/mongo/util/concurrency/remap_lock.h
new file mode 100644
index 00000000000..c45d0d3dafb
--- /dev/null
+++ b/src/mongo/util/concurrency/remap_lock.h
@@ -0,0 +1,31 @@
+// remap_lock.h
+
+/*
+ *    Copyright 2012 10gen Inc.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+// Used to synchronize thread creation in initAndListen() in mongod with
+// MemoryMappedFile::remapPrivateView() in Windows.  A dummy in mongos.
+// The functional constructor & destructor for mongod are in mmap_win.cpp.
+// The no-op constructor & destructor for mongos are in server.cpp.
+
+namespace mongo {
+
+    struct RemapLock {
+        RemapLock();
+        ~RemapLock();
+    };
+
+}
diff --git a/src/mongo/util/mmap_win.cpp b/src/mongo/util/mmap_win.cpp
index 2bf97930089..2a2c2f88337 100644
--- a/src/mongo/util/mmap_win.cpp
+++ b/src/mongo/util/mmap_win.cpp
@@ -22,6 +22,7 @@
 #include "../db/concurrency.h"
 #include "../db/memconcept.h"
 #include "mongo/util/timer.h"
+#include "mongo/util/concurrency/remap_lock.h"
 
 namespace mongo {
 
@@ -31,6 +32,17 @@ namespace mongo {
     MAdvise::MAdvise(void *,unsigned, Advice) { }
     MAdvise::~MAdvise() { }
 
+    // SERVER-2942 -- We do it this way because RemapLock is used in both mongod and mongos but
+    // we need different effects.  When called in mongod it needs to be a mutex and in mongos it
+    // needs to be a no-op.  This is the mongod version, the no-op mongos version is in server.cpp.
+    SimpleMutex _remapLock( "remapLock" );
+    RemapLock::RemapLock() {
+        _remapLock.lock();
+    }
+    RemapLock::~RemapLock() {
+        _remapLock.unlock();
+    }
+
     /** notification on unmapping so we can clear writable bits */
     void MemoryMappedFile::clearWritableBits(void *p) {
         for( unsigned i = ((size_t)p)/ChunkSize; i <= (((size_t)p)+len)/ChunkSize; i++ ) {
@@ -224,40 +236,29 @@ namespace mongo {
     void* MemoryMappedFile::remapPrivateView(void *oldPrivateAddr) {
         d.dbMutex.assertWriteLocked(); // short window where we are unmapped so must be exclusive
 
-        // the mapViewMutex is to assure we get the same address on the remap
-        scoped_lock lk(mapViewMutex);
+        RemapLock lk;   // Interlock with PortMessageServer::acceptedMP() to stop thread creation
 
         clearWritableBits(oldPrivateAddr);
-#if 1
-        // https://jira.mongodb.org/browse/SERVER-2942
-        DWORD old;
-        bool ok = VirtualProtect(oldPrivateAddr, (SIZE_T) len, PAGE_READONLY, &old);
-        if( !ok ) {
-            DWORD e = GetLastError();
-            log() << "VirtualProtect failed in remapPrivateView " << filename() << hex << oldPrivateAddr << ' ' << len << ' ' << errnoWithDescription(e) << endl;
-            verify(false);
-        }
-        return oldPrivateAddr;
-#else
         if( !UnmapViewOfFile(oldPrivateAddr) ) {
-            DWORD e = GetLastError();
-            log() << "UnMapViewOfFile failed " << filename() << ' ' << errnoWithDescription(e) << endl;
-            verify(false);
+            DWORD dosError = GetLastError();
+            log() << "UnMapViewOfFile for " << filename()
+                    << " failed with error " << errnoWithDescription( dosError ) << endl;
+            fassertFailed( 16147 );
         }
 
-        // we want the new address to be the same as the old address in case things keep pointers around (as namespaceindex does).
-        void *p = MapViewOfFileEx(maphandle, FILE_MAP_READ, 0, 0,
-                                  /*dwNumberOfBytesToMap 0 means to eof*/0 /*len*/,
-                                  oldPrivateAddr);
-        
-        if ( p == 0 ) {
-            DWORD e = GetLastError();
-            log() << "MapViewOfFileEx failed " << filename() << " " << errnoWithDescription(e) << endl;
-            verify(p);
+        void* newPrivateView = MapViewOfFileEx(
+                maphandle,          // file mapping handle
+                FILE_MAP_READ,      // access
+                0, 0,               // file offset, high and low
+                0,                  // bytes to map, 0 == all
+                oldPrivateAddr );   // we want the same address we had before
+        if ( 0 == newPrivateView ) {
+            DWORD dosError = GetLastError();
+            log() << "MapViewOfFileEx for " << filename()
+                    << " failed with error " << errnoWithDescription( dosError ) << endl;
         }
-        verify(p == oldPrivateAddr);
-        return p;
-#endif
+        fassert( 16148, newPrivateView == oldPrivateAddr );
+        return newPrivateView;
     }
 
     class WindowsFlushable : public MemoryMappedFile::Flushable {
diff --git a/src/mongo/util/net/message_server_port.cpp b/src/mongo/util/net/message_server_port.cpp
index 2dd0821cf3d..0b8c2c54725 100644
--- a/src/mongo/util/net/message_server_port.cpp
+++ b/src/mongo/util/net/message_server_port.cpp
@@ -27,6 +27,7 @@
 #include "../../db/cmdline.h"
 #include "../../db/lasterror.h"
 #include "../../db/stats/counters.h"
+#include "mongo/util/concurrency/remap_lock.h"
 #include "mongo/util/concurrency/ticketholder.h"
 
 #ifdef __linux__  // TODO: consider making this ifndef _WIN32
@@ -133,7 +134,16 @@ namespace mongo {
 
             try {
 #ifndef __linux__  // TODO: consider making this ifdef _WIN32
-                boost::thread thr( boost::bind( &pms::threadRun , p ) );
+                {
+#ifdef _WIN32
+                    // This Windows-only lock is to protect MemoryMappedFile::remapPrivateView ...
+                    //  it unmaps and remaps the private map and needs to get the previous address,
+                    //  and if we let a new thread get created between those calls, its thread
+                    //  stack could be created within that block, leading to an fassert ...
+                    RemapLock lk;
+#endif
+                    boost::thread thr( boost::bind( &pms::threadRun , p ) );
+                }
 #else
                 pthread_attr_t attrs;
                 pthread_attr_init(&attrs);