ReentrantLock 을 잘못 사용한 삽질 경험 > 구조를 잘 파악하고 사용하자!
무슨 짓을 저질렀나
n:n 채팅방을 만들면서 구현해야 하는 주요 기능들의 목록이 아래와 같았다.
- 한 유저가 채팅을 입력하면 서버에 등록된 모든 유저에게 전송
- 일대일 채팅
- 유저 지정 후 파일 전송
기능을 구현하고 서버와 연결한 클라이언트를 관리 및 이용하기 위해서
서버에서 ClientResisterManager라는 클래스를 만들고 안에 clientsMap이라는 Hashmap이라는 필드를 생성 후 이 필드를 관리해주는 메서드를 만들었다.
(String은 유저 이름, 그리고 UserManager는 소켓과 채팅수를 저장하는 클래스이다)
public class ClientResisterManager {
private HashMap<String, UserManager> clientsMap = new HashMap<>();
private final ReentrantLock L2 = new ReentrantLock();
public void resisterUser(String userName, UserManager userManager) {
L2.lock();
try {
clientsMap.put(userName, userManager);
} catch (Exception e) {
e.printStackTrace();
} finally {
L2.unlock();
}
}
public void removeUser(String userName) {
L2.lock();
try {
clientsMap.remove(userName);
} catch (Exception e) {
e.printStackTrace();
} finally {
L2.unlock();
}
}
public boolean containsUser(String userName) {
L2.lock();
try {
return clientsMap.containsKey(userName);
} finally {
L2.unlock();
}
}
public UserManager getUserManager(String userName) {
L2.lock();
try {
return clientsMap.get(userName);
} finally {
L2.unlock();
}
}
public HashMap<String, UserManager> getClientsMap() {
L2.lock();
try {
return this.clientsMap
} finally {
L2.unlock();
}
}
}이렇게 만든 클래스를 서버에서
유저의 패킷을 처리하는 ServerMessageController에서 사용하는데
(클래스의 코드가 길어 대표적으로 실수를 저지른 sendClientMessageToAllClient와 disconnectClientForRegisterState만 적어놓았다 다른 메서드도 같은 이 메서드와 같은 실수이다)
- 실수한 부분은 락에 대해 잘못된 이해로 인해
L1 LOCK으로 clientsMap에 대해 동시접근을 막았다고 생각했다.
하지만 피드백 받고 나서 어떤 지점이 잘못되었는지 알게 되었다.
Server의 전체적인 구조를 먼저 설명하자면
public class ServerStarter {
public void start() {
try {
ServerSocket serverSocket = new ServerSocket(7777);
System.out.println("[Sever Start]");
ClientResisterManager clientResisterManager = new ClientResisterManager();
while (true) {
// 클라이언트가 접속한 만큼 새로운 소켓이 생성되니 동시에 처리해줄 스레드 필요
Socket socket = serverSocket.accept();
System.out.println("[Client Connecting]");
ServerReceiver serverReceiver = new ServerReceiver(socket, clientResisterManager);
serverReceiver.setDaemon(true);
serverReceiver.start();
}
} catch (IOException e) {
e.printStackTrace();
}
}
}
어떻게 해결했는가
결과부터 말하자면
-
지금의 L1 Lock은 같은 메서드를 사용하는 경우에만 막아주는 것이다!
-
map이나 여러 객체 혹은 데이터를 저장하는 데이터는 직접 참조를 하지 못하게 하고 복사복을 만들어서 return해라! |
이다.
서버를 구동시키면서 각 유저가 연결될 때 마다 ServerReceiver 클래스의 인스턴스가 새로 생기고 클래스 안의 코드로 인해 serverMessageController 인스턴스도 새로 생기게 된다
다만, clientResisterManager 만큼은 유저마다 공유하게 된다
밑의 코드를 보면 L1 LOCK이 clientsMap을 보호하는 것 처럼 보이지만
실상은 그렇지 않다.
serverMessageController가 연결한 유저마다 새롭게 인스턴스가 생기니 클래스 안의 어떤 메서드를 사용하는지는 예측 불가능하다.
유저가 여러명 있다고 하고 밑의 두 메서드와 위의 ClientResisterManager 클래스의 메서드 중 getClientsMap()을 사용한다고 예를 들면 1번 유저(스레드)가 전체 채팅 메서드(sendClientMessageToAllClient)를 사용하고
2번 유저(스레드)가 서버와의 연결 종료 메서드(disconnectClientForRegisterState)를 사용하면 L2 Lock이 걸려있긴 하지만 clientsMap의 원본을 return받게 되고 삭제하게 된다면 스레드는 순서를 보장하지 않으니 1번 유저가 전체적으로 메세지를 보내려 했던 유저 명단 안에서 2번 유저가 누락될 수 있다.
@Slf4j
public class ServerMessageController {
private final ReentrantLock L1 = new ReentrantLock();
private byte[] packetToByte(HeaderPacket packet) {
byte[] header = packet.getHeaderBytes();
byte[] body = packet.getBodyBytes();
byte[] packetByte = new byte[header.length + body.length];
System.arraycopy(header, 0, packetByte, 0, header.length);
System.arraycopy(body, 0, packetByte, header.length, body.length);
log.debug("ServerMessageController's packetToByte method is called / packetType : {}, packetBytesLength : {}, packetBytes {}", packet.getPacketType(), packetByte.length, packetByte);
return packetByte;
}
public void sendClientMessageToAllClient(PacketForUserMessage messagePacket, ClientResisterManager clientResisterManager) {
String name = messagePacket.getName();
byte[] messageBytes = packetToByte(messagePacket);
log.debug("ServerMessageController's sendClientMessageToAllClient method is called in server / messageBytesLength: {}, messageBytes : {}", messageBytes.length, messageBytes);
try {
for (HashMap.Entry<String, UserManager> entry : clientResisterManager.getClientsCopyMap().entrySet()) {
UserManager userManager = entry.getValue();
OutputStream outputStream = userManager.getSocket().getOutputStream();
if (messagePacket.getName().equals(entry.getKey())) {
userManager.addChatCount();
continue;
}
outputStream.write(messageBytes);
outputStream.flush();
}
} catch (IOException e) {
clientResisterManager.removeUser(name);
}
}
public void disconnectClientForRegisterState(PacketForUserExit packetForUserExit, ClientResisterManager clientResisterManager) {
String exitUserName = packetForUserExit.getName();
log.debug("ServerMessageController's disconnectClient is called in server./ exitUserName : {}", exitUserName);
PacketForUserExitFromServer packetForUserExitFromServer = new PacketForUserExitFromServer(exitUserName, clientResisterManager.getUserManager(exitUserName).getChatCount());
log.debug("ServerMessageController's disconnectClient is called in server./ packetForUserExitFromServer : {}", packetForUserExitFromServer);
serverNotifyToAllForExitUser(packetForUserExitFromServer, clientResisterManager);
clientResisterManager.removeUser(exitUserName);
}
}따라서 clientResisterManager 클래스의 메서드의 코드를 hashmap의 복사본을 반환하는 코드로 바꿔서 문제를 해결했다.
public HashMap<String, UserManager> getClientsCopyMap() {
L2.lock();
try {
return new HashMap<>(this.clientsMap);
} finally {
L2.unlock();
}
}다른 차원의 문제 발생 > 해결
근데 이 경우에 공유자원의 동시 접근에 대한 문제는 해결이 가능하나, 다른 차원의 문제가 생긴다.
clientsMap의 복사본을 받아서 전체 채팅 메서드에 대한 코드가 진행 되는 중에 다른 유저가 서버와 접속을 종료한다면 OutputStream을 write()하는 쪽에서 문제가 될 것이다
따라서 코드를 이렇게 바꿔서 예외처리를 할 수 있게 만들었다
public void sendClientMessageToAllClient(PacketForUserMessage messagePacket, ClientResisterManager clientResisterManager) {
String name = messagePacket.getName();
byte[] messageBytes = packetToByte(messagePacket);
log.debug("ServerMessageController's sendClientMessageToAllClient method is called in server / messageBytesLength: {}, messageBytes : {}", messageBytes.length, messageBytes);
for (HashMap.Entry<String, UserManager> entry : clientResisterManager.getClientsCopyMap().entrySet()) {
UserManager userManager = entry.getValue();
try {
OutputStream outputStream = userManager.getSocket().getOutputStream();
if (name.equals(entry.getKey())) {
userManager.addChatCount();
continue;
}
outputStream.write(messageBytes);
outputStream.flush();
} catch (IOException e) {
clientResisterManager.removeUser(entry.getKey());
log.error("Error while sending message to client: " + entry.getKey(), e);
}