From c3495c8f3afbc0736ac184c1bbbb21214a85dda3 Mon Sep 17 00:00:00 2001 From: Enrico Giordani Date: Sat, 4 Jul 2015 12:08:41 +0200 Subject: [PATCH] [Fix] RFDMap was not thread safe. [Cleanup] Removed unused/commented out code, tabs->spaces. --- src/Win32_Interop/win32_rfdmap.cpp | 185 ++++++++++++++--------------- src/Win32_Interop/win32_rfdmap.h | 86 ++++++-------- 2 files changed, 123 insertions(+), 148 deletions(-) diff --git a/src/Win32_Interop/win32_rfdmap.cpp b/src/Win32_Interop/win32_rfdmap.cpp index e447cbcc..2375a6a6 100644 --- a/src/Win32_Interop/win32_rfdmap.cpp +++ b/src/Win32_Interop/win32_rfdmap.cpp @@ -22,139 +22,126 @@ #include "win32_types.h" - #include "win32_rfdmap.h" RFDMap& RFDMap::getInstance() { - static RFDMap instance; // Instantiated on first use. Guaranteed to be destroyed. + static RFDMap instance; // Instantiated on first use. Guaranteed to be destroyed. return instance; } -RFDMap::RFDMap() { maxRFD = minRFD; }; +RFDMap::RFDMap() { + InitializeCriticalSection(&mutex); + maxRFD = minRFD; +} RFD RFDMap::getNextRFDAvailable() { - if( RFDRecyclePool.empty() == false ) { - int RFD = RFDRecyclePool.front(); - RFDRecyclePool.pop(); - return RFD; - } else { - maxRFD = minRFD + (int)SocketToRFDMap.size() + (int)PosixFDToRFDMap.size(); - return maxRFD; - } + RFD rfd; + EnterCriticalSection(&mutex); + if (RFDRecyclePool.empty() == false) { + rfd = RFDRecyclePool.front(); + RFDRecyclePool.pop(); + } else { + maxRFD = minRFD + (int) SocketToRFDMap.size() + (int) PosixFDToRFDMap.size(); + rfd = maxRFD; + } + LeaveCriticalSection(&mutex); + return rfd; } RFD RFDMap::addSocket(SOCKET s) { - if (SocketToRFDMap.find(s) != SocketToRFDMap.end()) { - return invalidRFD; - } - - RFD rfd = getNextRFDAvailable(); - SocketToRFDMap[s] = rfd; - RFDToSocketMap[rfd] = s; - return rfd; + RFD rfd; + EnterCriticalSection(&mutex); + if (SocketToRFDMap.find(s) != SocketToRFDMap.end()) { + rfd = invalidRFD; + } else { + rfd = getNextRFDAvailable(); + SocketToRFDMap[s] = rfd; + RFDToSocketMap[rfd] = s; + } + LeaveCriticalSection(&mutex); + return rfd; } -void RFDMap::removeSocket(SOCKET s) { - S2RFDIterator mit = SocketToRFDMap.find(s); - if(mit == SocketToRFDMap.end()) { -// redisLog( REDIS_DEBUG, "RFDMap::removeSocket() - failed to find socket!" ); - return; +void RFDMap::removeSocket(SOCKET s) { + EnterCriticalSection(&mutex); + S2RFDIterator mit = SocketToRFDMap.find(s); + if (mit != SocketToRFDMap.end()) { + RFD rfd = (*mit).second; + RFDRecyclePool.push(rfd); + RFDToSocketMap.erase(rfd); + SocketToRFDMap.erase(s); } - RFD rfd = (*mit).second; - RFDRecyclePool.push(rfd); - RFDToSocketMap.erase(rfd); - SocketToRFDMap.erase(s); + LeaveCriticalSection(&mutex); } RFD RFDMap::addPosixFD(int posixFD) { - if (PosixFDToRFDMap.find(posixFD) != PosixFDToRFDMap.end()) { -// redisLog( REDIS_DEBUG, "RFDMap::addPosixFD() - posixFD already exists!" ); - return invalidRFD; - } - - RFD rfd = getNextRFDAvailable(); - PosixFDToRFDMap[posixFD] = rfd; - RFDToPosixFDMap[rfd] = posixFD; - return rfd; + RFD rfd; + EnterCriticalSection(&mutex); + if (PosixFDToRFDMap.find(posixFD) != PosixFDToRFDMap.end()) { + rfd = invalidRFD; + } else { + rfd = getNextRFDAvailable(); + PosixFDToRFDMap[posixFD] = rfd; + RFDToPosixFDMap[rfd] = posixFD; + } + LeaveCriticalSection(&mutex); + return rfd; } -void RFDMap::removePosixFD(int posixFD) { - PosixFD2RFDIterator mit = PosixFDToRFDMap.find(posixFD); - if(mit == PosixFDToRFDMap.end()) { -// redisLog( REDIS_DEBUG, "RFDMap::removePosixFD() - failed to find posix FD!" ); - return; +void RFDMap::removePosixFD(int posixFD) { + EnterCriticalSection(&mutex); + PosixFD2RFDIterator mit = PosixFDToRFDMap.find(posixFD); + if (mit != PosixFDToRFDMap.end()) { + RFD rfd = (*mit).second; + RFDRecyclePool.push(rfd); + RFDToPosixFDMap.erase(rfd); + PosixFDToRFDMap.erase(posixFD); } - RFD rfd = (*mit).second; - RFDRecyclePool.push(rfd); - RFDToPosixFDMap.erase(rfd); - PosixFDToRFDMap.erase(posixFD); + LeaveCriticalSection(&mutex); } SOCKET RFDMap::lookupSocket(RFD rfd) { - if (RFDToSocketMap.find(rfd) != RFDToSocketMap.end()) { - return RFDToSocketMap[rfd]; - } else { -// redisLog( REDIS_DEBUG, "RFDMap::lookupSocket() - failed to find socket!" ); - return INVALID_SOCKET; - } + SOCKET socket = INVALID_SOCKET; + EnterCriticalSection(&mutex); + if (RFDToSocketMap.find(rfd) != RFDToSocketMap.end()) { + socket = RFDToSocketMap[rfd]; + } + LeaveCriticalSection(&mutex); + return socket; } int RFDMap::lookupPosixFD(RFD rfd) { - if (RFDToPosixFDMap.find(rfd) != RFDToPosixFDMap.end()) { - return RFDToPosixFDMap[rfd]; - } else if (rfd >= 0 && rfd <= 2) { - return rfd; - } - else { -// redisLog( REDIS_DEBUG, "RFDMap::lookupPosixFD() - failed to find posix FD!" ); - return -1; - } + int posixFD = -1; + EnterCriticalSection(&mutex); + if (RFDToPosixFDMap.find(rfd) != RFDToPosixFDMap.end()) { + posixFD = RFDToPosixFDMap[rfd]; + } else if (rfd >= 0 && rfd <= 2) { + posixFD = rfd; + } + LeaveCriticalSection(&mutex); + return posixFD; } -RFD RFDMap::lookupRFD(SOCKET s) { - if (SocketToRFDMap.find(s) != SocketToRFDMap.end()) { - return SocketToRFDMap[s]; - } else { -// redisLog( REDIS_DEBUG, "RFDMap::lookupFD() - failed to map SOCKET to RFD!" ); - return invalidRFD; - } -} - -RFD RFDMap::lookupRFD(int posixFD) { - if (PosixFDToRFDMap.find(posixFD) != PosixFDToRFDMap.end()) { - return PosixFDToRFDMap[posixFD]; - } else { -// redisLog( REDIS_DEBUG, "RFDMap::lookupFD() - failed to map posixFD to RFD!" ); - return invalidRFD; - } -} - -RFD RFDMap::getMinRFD() { - return minRFD; -} - -RFD RFDMap::getMaxRFD() { - return maxRFD; -} - -bool RFDMap::SetSocketState( SOCKET s, RedisSocketState state ) -{ +bool RFDMap::SetSocketState(SOCKET s, RedisSocketState state) { + bool result = false; + EnterCriticalSection(&mutex); S2StateIterator sit = SocketToStateMap.find(s); - if(sit != SocketToStateMap.end() ) { + if (sit != SocketToStateMap.end()) { SocketToStateMap[s] = state; - return true; - } else { - return false; + result = true; } + LeaveCriticalSection(&mutex); + return result; } -bool RFDMap::GetSocketState( SOCKET s, RedisSocketState& state ) -{ +bool RFDMap::GetSocketState(SOCKET s, RedisSocketState& state) { + bool result = false; + EnterCriticalSection(&mutex); S2StateIterator sit = SocketToStateMap.find(s); - if(sit != SocketToStateMap.end() ) { + if (sit != SocketToStateMap.end()) { state = SocketToStateMap[s]; - return true; - } else { - return false; + result = true; } + LeaveCriticalSection(&mutex); + return result; } diff --git a/src/Win32_Interop/win32_rfdmap.h b/src/Win32_Interop/win32_rfdmap.h index 7625132e..227a0274 100644 --- a/src/Win32_Interop/win32_rfdmap.h +++ b/src/Win32_Interop/win32_rfdmap.h @@ -23,15 +23,12 @@ #pragma once #include - #define INCL_WINSOCK_API_PROTOTYPES 0 // Important! Do not include Winsock API definitions to avoid conflicts with API entry points defnied below. #include #include "ws2tcpip.h" - -//#include - #include #include + using namespace std; typedef struct { @@ -69,65 +66,56 @@ public: private: RFDMap(); - RFDMap(RFDMap const&); // Don't implement to guarantee singleton semantics - void operator=(RFDMap const&); // Don't implement to guarantee singleton semantics + RFDMap(RFDMap const&); // Don't implement to guarantee singleton semantics + void operator=(RFDMap const&); // Don't implement to guarantee singleton semantics private: - SocketToRFDMapType SocketToRFDMap; - SocketToStateMapType SocketToStateMap; + SocketToRFDMapType SocketToRFDMap; + SocketToStateMapType SocketToStateMap; PosixFDToRFDMapType PosixFDToRFDMap; - RFDToSocketMapType RFDToSocketMap; - RFDToPosixFDMapType RFDToPosixFDMap; - RFDRecyclePoolType RFDRecyclePool; - -public: - const static int minRFD = 3; // 0, 1 and 2 are reserved for stdin, stdout and stderr - RFD maxRFD; - const static int invalidRFD = -1; + RFDToSocketMapType RFDToSocketMap; + RFDToPosixFDMapType RFDToPosixFDMap; + RFDRecyclePoolType RFDRecyclePool; private: - /* Gets the next available Redis File Descriptor. Redis File Descriptors are always - non-negative integers, with the first three being reserved for stdin(0), - stdout(1) and stderr(2). */ - RFD getNextRFDAvailable(); + const static int minRFD = 3; // 0, 1 and 2 are reserved for stdin, stdout and stderr + RFD maxRFD; + CRITICAL_SECTION mutex; public: - /* Adds a socket to the socket map. Returns the redis file descriptor value for - the socket. Returns invalidRFD if the socket is already added to the - collection. */ - RFD addSocket(SOCKET s); + const static int invalidRFD = -1; - /* Removes a socket from the list of sockets. Also removes the associated - file descriptor. */ - void removeSocket(SOCKET s); +private: + /* Gets the next available Redis File Descriptor. Redis File Descriptors are always + non-negative integers, with the first three being reserved for stdin(0), + stdout(1) and stderr(2). */ + RFD getNextRFDAvailable(); - /* Adds a posixFD (used with low-level CRT posix file functions) to the posixFD map. Returns +public: + /* Adds a socket to the socket map. Returns the redis file descriptor value for + the socket. Returns invalidRFD if the socket is already added to the + collection. */ + RFD addSocket(SOCKET s); + + /* Removes a socket from the list of sockets. Also removes the associated + file descriptor. */ + void removeSocket(SOCKET s); + + /* Adds a posixFD (used with low-level CRT posix file functions) to the posixFD map. Returns the redis file descriptor value for the posixFD. Returns invalidRFD if the posicFD is already added to the collection. */ - RFD addPosixFD(int posixFD); + RFD addPosixFD(int posixFD); - /* Removes a socket from the list of sockets. Also removes the associated - file descriptor. */ - void removePosixFD(int posixFD); - - /* Returns the socket associated with a file descriptor. */ - SOCKET lookupSocket(RFD rfd); + /* Removes a socket from the list of sockets. Also removes the associated + file descriptor. */ + void removePosixFD(int posixFD); /* Returns the socket associated with a file descriptor. */ - int lookupPosixFD(RFD rfd); + SOCKET lookupSocket(RFD rfd); - /* Returns the RFD associated with a socket. */ - RFD lookupRFD(SOCKET s); + /* Returns the socket associated with a file descriptor. */ + int lookupPosixFD(RFD rfd); - /* Returns the RFD associated with a posix FD. */ - RFD lookupRFD(int posixFD); - - /* Returns the smallest RFD available */ - RFD getMinRFD(); - - /* Returns the largest FD allocated so far */ - RFD getMaxRFD(); - - bool SetSocketState( SOCKET s, RedisSocketState state ); - bool GetSocketState( SOCKET s, RedisSocketState& state ); + bool SetSocketState(SOCKET s, RedisSocketState state); + bool GetSocketState(SOCKET s, RedisSocketState& state); };