Skip to content

Commit c3495c8

Browse files
enricogiorenricogior
committed
[Fix] RFDMap was not thread safe.
[Cleanup] Removed unused/commented out code, tabs->spaces.
1 parent 7370c06 commit c3495c8

File tree

2 files changed

+121
-146
lines changed

2 files changed

+121
-146
lines changed

‎src/Win32_Interop/win32_rfdmap.cpp‎

Lines changed: 86 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -22,139 +22,126 @@
2222

2323

2424
#include "win32_types.h"
25-
2625
#include "win32_rfdmap.h"
2726

2827
RFDMap& RFDMap::getInstance() {
29-
static RFDMap instance; // Instantiated on first use. Guaranteed to be destroyed.
28+
static RFDMap instance; // Instantiated on first use. Guaranteed to be destroyed.
3029
return instance;
3130
}
3231

33-
RFDMap::RFDMap() { maxRFD = minRFD; };
32+
RFDMap::RFDMap() {
33+
InitializeCriticalSection(&mutex);
34+
maxRFD = minRFD;
35+
}
3436

3537
RFD RFDMap::getNextRFDAvailable() {
36-
if( RFDRecyclePool.empty() == false ) {
37-
int RFD = RFDRecyclePool.front();
38-
RFDRecyclePool.pop();
39-
return RFD;
40-
} else {
41-
maxRFD = minRFD + (int)SocketToRFDMap.size() + (int)PosixFDToRFDMap.size();
42-
return maxRFD;
43-
}
38+
RFD rfd;
39+
EnterCriticalSection(&mutex);
40+
if (RFDRecyclePool.empty() == false) {
41+
rfd = RFDRecyclePool.front();
42+
RFDRecyclePool.pop();
43+
} else {
44+
maxRFD = minRFD + (int) SocketToRFDMap.size() + (int) PosixFDToRFDMap.size();
45+
rfd = maxRFD;
46+
}
47+
LeaveCriticalSection(&mutex);
48+
return rfd;
4449
}
4550

4651
RFD RFDMap::addSocket(SOCKET s) {
47-
if (SocketToRFDMap.find(s) != SocketToRFDMap.end()) {
48-
return invalidRFD;
49-
}
50-
51-
RFD rfd = getNextRFDAvailable();
52-
SocketToRFDMap[s] = rfd;
53-
RFDToSocketMap[rfd] = s;
54-
return rfd;
52+
RFD rfd;
53+
EnterCriticalSection(&mutex);
54+
if (SocketToRFDMap.find(s) != SocketToRFDMap.end()) {
55+
rfd = invalidRFD;
56+
} else {
57+
rfd = getNextRFDAvailable();
58+
SocketToRFDMap[s] = rfd;
59+
RFDToSocketMap[rfd] = s;
60+
}
61+
LeaveCriticalSection(&mutex);
62+
return rfd;
5563
}
5664

57-
void RFDMap::removeSocket(SOCKET s) {
58-
S2RFDIterator mit = SocketToRFDMap.find(s);
59-
if(mit == SocketToRFDMap.end()) {
60-
// redisLog( REDIS_DEBUG, "RFDMap::removeSocket() - failed to find socket!" );
61-
return;
65+
void RFDMap::removeSocket(SOCKET s) {
66+
EnterCriticalSection(&mutex);
67+
S2RFDIterator mit = SocketToRFDMap.find(s);
68+
if (mit != SocketToRFDMap.end()) {
69+
RFD rfd = (*mit).second;
70+
RFDRecyclePool.push(rfd);
71+
RFDToSocketMap.erase(rfd);
72+
SocketToRFDMap.erase(s);
6273
}
63-
RFD rfd = (*mit).second;
64-
RFDRecyclePool.push(rfd);
65-
RFDToSocketMap.erase(rfd);
66-
SocketToRFDMap.erase(s);
74+
LeaveCriticalSection(&mutex);
6775
}
6876

6977
RFD RFDMap::addPosixFD(int posixFD) {
70-
if (PosixFDToRFDMap.find(posixFD) != PosixFDToRFDMap.end()) {
71-
// redisLog( REDIS_DEBUG, "RFDMap::addPosixFD() - posixFD already exists!" );
72-
return invalidRFD;
73-
}
74-
75-
RFD rfd = getNextRFDAvailable();
76-
PosixFDToRFDMap[posixFD] = rfd;
77-
RFDToPosixFDMap[rfd] = posixFD;
78-
return rfd;
78+
RFD rfd;
79+
EnterCriticalSection(&mutex);
80+
if (PosixFDToRFDMap.find(posixFD) != PosixFDToRFDMap.end()) {
81+
rfd = invalidRFD;
82+
} else {
83+
rfd = getNextRFDAvailable();
84+
PosixFDToRFDMap[posixFD] = rfd;
85+
RFDToPosixFDMap[rfd] = posixFD;
86+
}
87+
LeaveCriticalSection(&mutex);
88+
return rfd;
7989
}
8090

81-
void RFDMap::removePosixFD(int posixFD) {
82-
PosixFD2RFDIterator mit = PosixFDToRFDMap.find(posixFD);
83-
if(mit == PosixFDToRFDMap.end()) {
84-
// redisLog( REDIS_DEBUG, "RFDMap::removePosixFD() - failed to find posix FD!" );
85-
return;
91+
void RFDMap::removePosixFD(int posixFD) {
92+
EnterCriticalSection(&mutex);
93+
PosixFD2RFDIterator mit = PosixFDToRFDMap.find(posixFD);
94+
if (mit != PosixFDToRFDMap.end()) {
95+
RFD rfd = (*mit).second;
96+
RFDRecyclePool.push(rfd);
97+
RFDToPosixFDMap.erase(rfd);
98+
PosixFDToRFDMap.erase(posixFD);
8699
}
87-
RFD rfd = (*mit).second;
88-
RFDRecyclePool.push(rfd);
89-
RFDToPosixFDMap.erase(rfd);
90-
PosixFDToRFDMap.erase(posixFD);
100+
LeaveCriticalSection(&mutex);
91101
}
92102

93103
SOCKET RFDMap::lookupSocket(RFD rfd) {
94-
if (RFDToSocketMap.find(rfd) != RFDToSocketMap.end()) {
95-
return RFDToSocketMap[rfd];
96-
} else {
97-
// redisLog( REDIS_DEBUG, "RFDMap::lookupSocket() - failed to find socket!" );
98-
return INVALID_SOCKET;
99-
}
104+
SOCKET socket = INVALID_SOCKET;
105+
EnterCriticalSection(&mutex);
106+
if (RFDToSocketMap.find(rfd) != RFDToSocketMap.end()) {
107+
socket = RFDToSocketMap[rfd];
108+
}
109+
LeaveCriticalSection(&mutex);
110+
return socket;
100111
}
101112

102113
int RFDMap::lookupPosixFD(RFD rfd) {
103-
if (RFDToPosixFDMap.find(rfd) != RFDToPosixFDMap.end()) {
104-
return RFDToPosixFDMap[rfd];
105-
} else if (rfd >= 0 && rfd <= 2) {
106-
return rfd;
107-
}
108-
else {
109-
// redisLog( REDIS_DEBUG, "RFDMap::lookupPosixFD() - failed to find posix FD!" );
110-
return -1;
111-
}
112-
}
113-
114-
RFD RFDMap::lookupRFD(SOCKET s) {
115-
if (SocketToRFDMap.find(s) != SocketToRFDMap.end()) {
116-
return SocketToRFDMap[s];
117-
} else {
118-
// redisLog( REDIS_DEBUG, "RFDMap::lookupFD() - failed to map SOCKET to RFD!" );
119-
return invalidRFD;
120-
}
121-
}
122-
123-
RFD RFDMap::lookupRFD(int posixFD) {
124-
if (PosixFDToRFDMap.find(posixFD) != PosixFDToRFDMap.end()) {
125-
return PosixFDToRFDMap[posixFD];
126-
} else {
127-
// redisLog( REDIS_DEBUG, "RFDMap::lookupFD() - failed to map posixFD to RFD!" );
128-
return invalidRFD;
129-
}
130-
}
131-
132-
RFD RFDMap::getMinRFD() {
133-
return minRFD;
134-
}
135-
136-
RFD RFDMap::getMaxRFD() {
137-
return maxRFD;
114+
int posixFD = -1;
115+
EnterCriticalSection(&mutex);
116+
if (RFDToPosixFDMap.find(rfd) != RFDToPosixFDMap.end()) {
117+
posixFD = RFDToPosixFDMap[rfd];
118+
} else if (rfd >= 0 && rfd <= 2) {
119+
posixFD = rfd;
120+
}
121+
LeaveCriticalSection(&mutex);
122+
return posixFD;
138123
}
139124

140-
bool RFDMap::SetSocketState( SOCKET s, RedisSocketState state )
141-
{
125+
bool RFDMap::SetSocketState(SOCKET s, RedisSocketState state) {
126+
bool result = false;
127+
EnterCriticalSection(&mutex);
142128
S2StateIterator sit = SocketToStateMap.find(s);
143-
if(sit != SocketToStateMap.end() ) {
129+
if (sit != SocketToStateMap.end()) {
144130
SocketToStateMap[s] = state;
145-
return true;
146-
} else {
147-
return false;
131+
result = true;
148132
}
133+
LeaveCriticalSection(&mutex);
134+
return result;
149135
}
150136

151-
bool RFDMap::GetSocketState( SOCKET s, RedisSocketState& state )
152-
{
137+
bool RFDMap::GetSocketState(SOCKET s, RedisSocketState& state) {
138+
bool result = false;
139+
EnterCriticalSection(&mutex);
153140
S2StateIterator sit = SocketToStateMap.find(s);
154-
if(sit != SocketToStateMap.end() ) {
141+
if (sit != SocketToStateMap.end()) {
155142
state = SocketToStateMap[s];
156-
return true;
157-
} else {
158-
return false;
143+
result = true;
159144
}
145+
LeaveCriticalSection(&mutex);
146+
return result;
160147
}

‎src/Win32_Interop/win32_rfdmap.h‎

Lines changed: 35 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -23,15 +23,12 @@
2323
#pragma once
2424

2525
#include <errno.h>
26-
2726
#define INCL_WINSOCK_API_PROTOTYPES 0 // Important! Do not include Winsock API definitions to avoid conflicts with API entry points defnied below.
2827
#include <WinSock2.h>
2928
#include "ws2tcpip.h"
30-
31-
//#include <io.h>
32-
3329
#include <map>
3430
#include <queue>
31+
3532
using namespace std;
3633

3734
typedef struct {
@@ -69,65 +66,56 @@ class RFDMap {
6966

7067
private:
7168
RFDMap();
72-
RFDMap(RFDMap const&); // Don't implement to guarantee singleton semantics
73-
void operator=(RFDMap const&); // Don't implement to guarantee singleton semantics
69+
RFDMap(RFDMap const&); // Don't implement to guarantee singleton semantics
70+
void operator=(RFDMap const&); // Don't implement to guarantee singleton semantics
7471

7572
private:
76-
SocketToRFDMapType SocketToRFDMap;
77-
SocketToStateMapType SocketToStateMap;
73+
SocketToRFDMapType SocketToRFDMap;
74+
SocketToStateMapType SocketToStateMap;
7875
PosixFDToRFDMapType PosixFDToRFDMap;
79-
RFDToSocketMapType RFDToSocketMap;
80-
RFDToPosixFDMapType RFDToPosixFDMap;
81-
RFDRecyclePoolType RFDRecyclePool;
76+
RFDToSocketMapType RFDToSocketMap;
77+
RFDToPosixFDMapType RFDToPosixFDMap;
78+
RFDRecyclePoolType RFDRecyclePool;
8279

83-
public:
84-
const static int minRFD = 3; // 0, 1 and 2 are reserved for stdin, stdout and stderr
80+
private:
81+
const static int minRFD = 3; // 0, 1 and 2 are reserved for stdin, stdout and stderr
8582
RFD maxRFD;
86-
const static int invalidRFD = -1;
83+
CRITICAL_SECTION mutex;
84+
85+
public:
86+
const static int invalidRFD = -1;
8787

8888
private:
89-
/* Gets the next available Redis File Descriptor. Redis File Descriptors are always
90-
non-negative integers, with the first three being reserved for stdin(0),
91-
stdout(1) and stderr(2). */
92-
RFD getNextRFDAvailable();
89+
/* Gets the next available Redis File Descriptor. Redis File Descriptors are always
90+
non-negative integers, with the first three being reserved for stdin(0),
91+
stdout(1) and stderr(2). */
92+
RFD getNextRFDAvailable();
9393

9494
public:
95-
/* Adds a socket to the socket map. Returns the redis file descriptor value for
96-
the socket. Returns invalidRFD if the socket is already added to the
97-
collection. */
98-
RFD addSocket(SOCKET s);
95+
/* Adds a socket to the socket map. Returns the redis file descriptor value for
96+
the socket. Returns invalidRFD if the socket is already added to the
97+
collection. */
98+
RFD addSocket(SOCKET s);
9999

100-
/* Removes a socket from the list of sockets. Also removes the associated
101-
file descriptor. */
102-
void removeSocket(SOCKET s);
100+
/* Removes a socket from the list of sockets. Also removes the associated
101+
file descriptor. */
102+
void removeSocket(SOCKET s);
103103

104-
/* Adds a posixFD (used with low-level CRT posix file functions) to the posixFD map. Returns
104+
/* Adds a posixFD (used with low-level CRT posix file functions) to the posixFD map. Returns
105105
the redis file descriptor value for the posixFD. Returns invalidRFD if the posicFD is already
106106
added to the collection. */
107-
RFD addPosixFD(int posixFD);
108-
109-
/* Removes a socket from the list of sockets. Also removes the associated
110-
file descriptor. */
111-
void removePosixFD(int posixFD);
107+
RFD addPosixFD(int posixFD);
112108

113-
/* Returns the socket associated with a file descriptor. */
114-
SOCKET lookupSocket(RFD rfd);
109+
/* Removes a socket from the list of sockets. Also removes the associated
110+
file descriptor. */
111+
void removePosixFD(int posixFD);
115112

116113
/* Returns the socket associated with a file descriptor. */
117-
int lookupPosixFD(RFD rfd);
118-
119-
/* Returns the RFD associated with a socket. */
120-
RFD lookupRFD(SOCKET s);
114+
SOCKET lookupSocket(RFD rfd);
121115

122-
/* Returns the RFD associated with a posix FD. */
123-
RFD lookupRFD(int posixFD);
124-
125-
/* Returns the smallest RFD available */
126-
RFD getMinRFD();
127-
128-
/* Returns the largest FD allocated so far */
129-
RFD getMaxRFD();
116+
/* Returns the socket associated with a file descriptor. */
117+
int lookupPosixFD(RFD rfd);
130118

131-
bool SetSocketState( SOCKET s, RedisSocketState state );
132-
bool GetSocketState( SOCKET s, RedisSocketState& state );
119+
bool SetSocketState(SOCKET s, RedisSocketState state);
120+
bool GetSocketState(SOCKET s, RedisSocketState& state);
133121
};

0 commit comments

Comments
 (0)