fix(chat): 채팅방 동시 생성 문제 해결 (#290)

* fix(db): 테스트 환경에서 초기 데이터 값이 들어가는 부분 수정

개요 및 수정사항

- 현재 초기에 삽입되는 데이터 sql의 파일명이 init.sql로 되어있어, sql
init never를 설정해줘도 테스트 실행 시에 자동으로 스프링에 삽입시킨다.
따라서 data.sql로 바꿔준다. 추가로, test시에 init mode를 never로
바꿔준다.

* fix(chat): 채팅방 존재 체크 JPQL 수정

개요

- 현재 채팅방 존재의 JPQL이 잘못되어 있어 올바른 체킹이 이루어지지 않고
있다.
- 기존에는 cm.member의 같은 변수로 두개의 값이랑 같은지 비교하므로
성립이 안됨
- 따라서, 따라서, JPQL 쿼리를 수정하여 각 멤버를 별도의 변수(cm1, cm2)로 매핑하고,
각각 :member1, :member2와 비교하도록 변경함. + 싱글 채팅방인 경우
- 이러면 채팅방 중에 member 둘 다 있고, 싱글 채팅방인 경우만 가져오므로
해결이 된다.

* fix(chat): 채팅방 동시 생성 경쟁 방지

개요

- 1:1 채팅을 둘이 동시에 만드는 경우에, 둘 다 생겨서 문제가 생긴다. 이를 방지하기 위해, 경쟁 조건을 방지해준다.

수정 사항

- 기존 수연님이 작성해주신 RedisLockChatServiceFacade.java의 코드를
재활용하기 편하게 바꿈
  - 우선 락 이름을 외부에서 알아서 넣어주도록 바꿈
  - executeWithLock을 오버로딩해서, Runnable (리턴 값없는 경우),
Callable (리턴 값 있는 경우)의 경우로 나누어서 함수를 작성함 -> 참고로
이 둘은 쓰레드 태스크 실행 시에 사용
  - createSingleChatroom과 기존 함수에서 이 바꾼 함수 사용
  - 락 이름 외부에서 변수로 넣어줌
    - generateSingleChatroomLockName은 memberId를 기준으로 낮은 id, 높은
id 순으로 두어 획일화된 락네임 설정
- 경쟁조건 테스트를 위해 간단한 테스트 작성함.
  - 유저 2명이서 동시에 채팅방생성을 해봐서, 생성된 채팅방 개수 1개인지
확인
  - 둘 중 하나는 오류로 실패했는지 확인

src/main/java/com/dife/api/repository/ChatroomRepository.java

@@ -15,7 +15,10 @@ public interface ChatroomRepository extends JpaRepository<Chatroom, Long> {
 	Boolean existsByName(String name);
 
 	@Query(
-			"SELECT COUNT(c) > 0 FROM Chatroom c JOIN c.members cm WHERE cm.member = :member1 AND cm.member = :member2 AND c.chatroomType = :chatroomType")
+			"SELECT COUNT(c) > 0 FROM Chatroom c "
+					+ "JOIN c.members cm1 ON cm1.member = :member1 "
+					+ "JOIN c.members cm2 ON cm2.member = :member2 "
+					+ "WHERE c.chatroomType = :chatroomType")
 	Boolean existsSingleChatroomByMembers(
 			@Param("member1") Member member1,
 			@Param("member2") Member member2,

src/main/java/com/dife/api/service/ChatService.java

@@ -107,7 +107,8 @@ public void enter(
 		String username = member.getUsername();
 
 		ChatroomSetting setting = chatroom.getChatroomSetting();
-		chatServiceFacade.increase(chatroomId);
+		chatServiceFacade.executeWithLock(
+				chatroomId.toString(), () -> chatroomService.increase(chatroomId));
 
 		Set<ChatroomMember> chatroomMembers = chatroom.getMembers();
 		chatroomMembers.add(chatroomService.createChatroomMember(chatroom, member));
@@ -243,7 +244,10 @@ private void handleExit(
 		Set<Member> memberSet = chatroomService.validChatroomMembers(chatroom);
 		if (!memberSet.contains(member)) throw new AccessDeniedException("해당 채팅방에 접근 권한이 없습니다.");
 
-		if (chatroom.getChatroomType() == GROUP) chatServiceFacade.decrease(chatroomId);
+		if (chatroom.getChatroomType() == GROUP) {
+			chatServiceFacade.executeWithLock(
+					chatroomId.toString(), () -> chatroomService.decrease(chatroomId));
+		}
 
 		ChatroomMember chatroomMember =
 				chatroomMemberRepository

src/main/java/com/dife/api/service/ChatroomService.java

@@ -35,29 +35,28 @@ public class ChatroomService {
 	private final ChatRepository chatRepository;
 
 	private final BlockService blockService;
-
 	private final ModelMapper modelMapper;
 
 	@Autowired
 	@Qualifier("chatroomModelMapper")
 	private ModelMapper chatroomModelMapper;
 
 	private final FileService fileService;
+	private final RedisLockChatServiceFacade redisLockChatServiceFacade;
+	private final MemberService memberService;
 
 	public Boolean isDuplicate(String name) {
 		return chatroomRepository.existsByName(name);
 	}
 
 	public List<ChatroomResponseDto> getChatrooms(ChatroomType type, String memberEmail) {
-
 		Member member =
 				memberRepository.findByEmail(memberEmail).orElseThrow(MemberNotFoundException::new);
 
 		List<ChatroomMember> chatroomMembers = chatroomMemberRepository.findAllByMember(member);
 
 		List<Chatroom> chatrooms;
 		if (type == ChatroomType.SINGLE) {
-
 			chatrooms =
 					chatroomMembers.stream()
 							.filter(
@@ -146,7 +145,8 @@ public ChatroomResponseDto createChatroom(
 						isPublic,
 						password);
 			case SINGLE:
-				return createSingleChatroom(toMemberId, memberEmail);
+				Member currentMember = memberService.getMemberEntityByEmail(memberEmail);
+				return createSingleChatroom(toMemberId, currentMember.getId());
 		}
 		throw new ChatroomException("유효한 채팅방 생성 접근이 아닙니다!");
 	}
@@ -336,35 +336,47 @@ public GroupChatroomResponseDto update(
 				chatroom, profileImg, setting, maxCount, purposes, hobbies, languages, isPublic, password);
 	}
 
-	public SingleChatroomResponseDto createSingleChatroom(
-			Long toMemberId, String currentMemberEmail) {
-		Member currentMember =
-				memberRepository.findByEmail(currentMemberEmail).orElseThrow(MemberNotFoundException::new);
-		Member otherMember =
-				memberRepository.findById(toMemberId).orElseThrow(MemberNotFoundException::new);
+	private String generateSingleChatroomLockName(Long memberId1, Long memberId2) {
+		Long smallerId = Math.min(memberId1, memberId2);
+		Long largerId = Math.max(memberId1, memberId2);
+		return String.format("singleChatroom:%d:%d", smallerId, largerId);
+	}
 
-		Set<Member> blockedMembers = blockService.getBlackSet(currentMember);
+	public SingleChatroomResponseDto createSingleChatroom(Long toMemberId, Long currentMemberId) {
+		String lockName = generateSingleChatroomLockName(toMemberId, currentMemberId);
+		Chatroom newChatroom =
+				redisLockChatServiceFacade.executeWithLock(
+						lockName,
+						() -> {
+							Member currentMember = memberService.getMemberEntityById(currentMemberId);
+							Member otherMember = memberService.getMemberEntityById(toMemberId);
 
-		if (blockedMembers.contains(otherMember))
-			throw new MemberException("차단된 사용자에게는 일대일 채팅을 보낼 수 없습니다!");
+							Set<Member> blockedMembers = blockService.getBlackSet(currentMember);
+							if (blockedMembers.contains(otherMember)) {
+								throw new MemberException("차단된 사용자에게는 일대일 채팅을 보낼 수 없습니다!");
+							}
 
-		if (chatroomRepository.existsSingleChatroomByMembers(
-				currentMember, otherMember, ChatroomType.SINGLE))
-			throw new SingleChatroomCreateDuplicateException();
+							if (chatroomRepository.existsSingleChatroomByMembers(
+									currentMember, otherMember, ChatroomType.SINGLE)) {
+								throw new SingleChatroomCreateDuplicateException();
+							}
 
-		Chatroom chatroom = new Chatroom();
-		chatroom.setChatroomType(ChatroomType.SINGLE);
+							Chatroom chatroom = new Chatroom();
+							chatroom.setChatroomType(ChatroomType.SINGLE);
+							chatroomRepository.save(chatroom);
 
-		chatroomRepository.save(chatroom);
-		Set<ChatroomMember> chatroomMembers = new HashSet<>();
-		chatroomMembers.add(createChatroomMember(chatroom, currentMember));
-		chatroomMembers.add(createChatroomMember(chatroom, otherMember));
-		chatroom.setMembers(chatroomMembers);
+							Set<ChatroomMember> chatroomMembers = new HashSet<>();
+							chatroomMembers.add(createChatroomMember(chatroom, currentMember));
+							chatroomMembers.add(createChatroomMember(chatroom, otherMember));
+							chatroom.setMembers(chatroomMembers);
+
+							return chatroom;
+						});
 
 		SingleChatroomResponseDto responseDto =
-				chatroomModelMapper.map(chatroom, SingleChatroomResponseDto.class);
+				chatroomModelMapper.map(newChatroom, SingleChatroomResponseDto.class);
 
-		changeChatroomMemberToMemberSet(chatroom, responseDto);
+		changeChatroomMemberToMemberSet(newChatroom, responseDto);
 		return responseDto;
 	}
 
@@ -379,7 +391,6 @@ public ChatroomMember createChatroomMember(Chatroom chatroom, Member member) {
 	}
 
 	public SingleChatroomResponseDto getSingleChatroom(Long id, String memberEmail) {
-
 		Chatroom chatroom = chatroomRepository.findById(id).orElseThrow(ChatroomNotFoundException::new);
 
 		Member member =

src/main/java/com/dife/api/service/RedisLockChatServiceFacade.java

@@ -1,51 +1,58 @@
 package com.dife.api.service;
 
-import com.dife.api.model.*;
+import java.util.concurrent.Callable;
 import java.util.concurrent.TimeUnit;
 import lombok.RequiredArgsConstructor;
 import lombok.extern.slf4j.Slf4j;
 import org.redisson.api.RLock;
 import org.redisson.api.RedissonClient;
-import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Component;
-import org.springframework.transaction.annotation.Transactional;
 
 @Component
 @RequiredArgsConstructor
 @Slf4j
 public class RedisLockChatServiceFacade {
+	private final RedissonClient redissonClient;
 
-	@Autowired private RedissonClient redissonClient;
-	@Autowired private ChatroomService chatroomService;
+	private static final long WAIT_TIME = 10L;
+	private static final long LEASE_TIME = 1L;
 
-	private static final long WAITTIME = 10L;
-	private static final long LEASETIME = 1L;
-
-	@Transactional
-	public void increase(Long chatroomId) {
-		executeWithLock(chatroomId, () -> chatroomService.increase(chatroomId));
-	}
-
-	@Transactional
-	public void decrease(Long chatroomId) {
-		executeWithLock(chatroomId, () -> chatroomService.decrease(chatroomId));
+	public void executeWithLock(String lockName, Runnable action) {
+		RLock lock = redissonClient.getLock(lockName);
+		try {
+			if (lock.tryLock(WAIT_TIME, LEASE_TIME, TimeUnit.SECONDS)) {
+				action.run();
+			}
+		} catch (InterruptedException e) {
+			Thread.currentThread().interrupt();
+			throw new RuntimeException("Interrupted during lock acquisition", e);
+		} finally {
+			releaseLock(lock);
+		}
 	}
 
-	private void executeWithLock(Long chatroomId, Runnable action) {
-		RLock lock = redissonClient.getLock("lock:" + chatroomId.toString());
+	public <T> T executeWithLock(String lockName, Callable<T> action) {
+		RLock lock = redissonClient.getLock(lockName);
 		try {
-			boolean available = lock.tryLock(WAITTIME, LEASETIME, TimeUnit.SECONDS);
-			if (!available) {
-				return;
+			if (lock.tryLock(WAIT_TIME, LEASE_TIME, TimeUnit.SECONDS)) {
+				return action.call();
+			} else {
+				return null;
 			}
-			action.run();
 		} catch (InterruptedException e) {
 			Thread.currentThread().interrupt();
 			throw new RuntimeException("Interrupted during lock acquisition", e);
+		} catch (Exception e) {
+			log.error("Exception during locked execution", e);
+			throw new RuntimeException("Exception during locked execution", e);
 		} finally {
-			if (lock.isLocked() && lock.isHeldByCurrentThread()) {
-				lock.unlock();
-			}
+			releaseLock(lock);
+		}
+	}
+
+	private void releaseLock(RLock lock) {
+		if (lock.isLocked() && lock.isHeldByCurrentThread()) {
+			lock.unlock();
 		}
 	}
 }

src/main/resources/application-local.yml

@@ -9,7 +9,7 @@ spring:
   sql:
     init:
       mode: never
-      data-locations: classpath:import.sql
+      data-locations: classpath:data.sql
 
   aws:
     access-key: ${AWS_ACCESS_KEY_ID}

src/test/java/com/dife/api/config/LocalRedisConfig.java

@@ -2,9 +2,11 @@
 
 import org.junit.jupiter.api.extension.BeforeAllCallback;
 import org.junit.jupiter.api.extension.ExtensionContext;
+import org.springframework.boot.test.context.TestConfiguration;
 import org.testcontainers.containers.GenericContainer;
 import org.testcontainers.utility.DockerImageName;
 
+@TestConfiguration
 public class LocalRedisConfig implements BeforeAllCallback {
 	private static final String REDIS_IMAGE = "redis:7.0.8-alpine";
 	private static final int REDIS_PORT = 6379;

src/test/java/com/dife/api/service/ChatroomServiceTest.java

@@ -0,0 +1,85 @@
+package com.dife.api.service;
+
+import static org.hibernate.validator.internal.util.Contracts.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import com.dife.api.config.LocalRedisConfig;
+import com.dife.api.model.Chatroom;
+import com.dife.api.model.Member;
+import com.dife.api.model.dto.SingleChatroomResponseDto;
+import com.dife.api.repository.ChatroomRepository;
+import com.dife.api.repository.MemberRepository;
+import java.util.List;
+import java.util.concurrent.*;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.boot.test.mock.mockito.MockBean;
+import org.springframework.mail.javamail.JavaMailSender;
+import org.springframework.test.context.ActiveProfiles;
+import software.amazon.awssdk.services.s3.S3Client;
+import software.amazon.awssdk.services.s3.presigner.S3Presigner;
+
+@SpringBootTest
+@ActiveProfiles("test")
+@ExtendWith(LocalRedisConfig.class)
+public class ChatroomServiceTest {
+	@MockBean private JavaMailSender javaMailSender;
+	@MockBean private S3Client s3Client;
+	@MockBean private S3Presigner s3Presigner;
+	@Autowired private ChatroomService chatroomService;
+	@Autowired private ChatroomRepository chatroomRepository;
+	@Autowired private MemberRepository memberRepository;
+
+	@Test
+	public void createSingleChatroom_ShouldCreate1_WhenInRaceCondition() {
+		Member member1 = new Member();
+		member1.setEmail("[email protected]");
+		memberRepository.save(member1);
+
+		Member member2 = new Member();
+		member2.setEmail("[email protected]");
+		memberRepository.save(member2);
+
+		Callable<SingleChatroomResponseDto> task1 =
+				() -> chatroomService.createSingleChatroom(member1.getId(), member2.getId());
+		Callable<SingleChatroomResponseDto> task2 =
+				() -> chatroomService.createSingleChatroom(member2.getId(), member1.getId());
+
+		ExecutorService executor = Executors.newFixedThreadPool(2);
+
+		Future<SingleChatroomResponseDto> future1 = executor.submit(task1);
+		Future<SingleChatroomResponseDto> future2 = executor.submit(task2);
+
+		SingleChatroomResponseDto dto1 = null;
+		SingleChatroomResponseDto dto2 = null;
+		Throwable exception1 = null;
+		Throwable exception2 = null;
+
+		try {
+			dto1 = future1.get();
+		} catch (ExecutionException | InterruptedException e) {
+			exception1 = e.getCause();
+		}
+
+		try {
+			dto2 = future2.get();
+		} catch (ExecutionException | InterruptedException e) {
+			exception2 = e.getCause();
+		}
+
+		executor.shutdown();
+
+		List<Chatroom> chatrooms = chatroomRepository.findAll();
+		assertEquals(1, chatrooms.size(), "Only one chatroom should be created");
+
+		boolean oneSuccessOneFailure =
+				(dto1 != null && exception2 instanceof RuntimeException)
+						|| (dto2 != null && exception1 instanceof RuntimeException);
+
+		assertTrue(
+				oneSuccessOneFailure,
+				"One task should succeed and the other should fail with SingleChatroomCreateDuplicateException");
+	}
+}

src/test/resources/application.yml

@@ -9,12 +9,11 @@ spring:
     username: root
     password: password
   jpa:
-    show-sql: true
     defer-datasource-initialization: true
 
   sql:
     init:
-      mode: always
+      mode: never
 
   main:
     allow-bean-definition-overriding: true