fix: abtest 데드락 이슈 발견/해결 #1075
fix: abtest 데드락 이슈 발견/해결 #1075
Conversation
There was a problem hiding this comment.
주말인데도 제 코드에서 발생한 문제를 해결하고자 소중한 시간 써주셔서 감사합니다.. 😭🙇♂️
중복되지 않는데 메소드로 분리한 이유는 로직만 나열되어있으면 가독성이 떨어져서 이를 위해 진행했었습니다.
도움이 될지 모르겠지만 #843 에 간단하게나마 로직 설명을 기록해두었습니다.
| // 이 값은 어디에쓰이는지? | ||
| UserAgentInfo userAgentInfo, |
There was a problem hiding this comment.
기기 정보 저장에 사용됩니다.
Device 엔티티의 model 및 type에 사용자의 기기 정보를 저장합니다.
저장 목적: 어드민 페이지에서의 사용자 실험군 수동 편입 시 기기 특정을 위해 기기 정보 제공
| // -- syncCacheCount 시작 | ||
| // abTest 변수들 중 Redis에 올라와있는 변수를 가져온다. | ||
| List<AbtestVariableCount> cachedCount = abtest.getAbtestVariables().stream() | ||
| .map(AbtestVariable::getId) | ||
| .filter(Objects::nonNull) | ||
| .map(abtestVariableCountRedisRepository::findById) | ||
| .filter(Optional::isPresent) | ||
| .map(Optional::get) | ||
| .toList(); | ||
|
|
||
| // Redis에 캐싱되어있는 count 값을 RDB로 동기화시킨다. | ||
| for (AbtestVariableCount abtestVariableCount : cachedCount) { | ||
| // RDB에서 abtestVariable 값을 영속성 컨텍스트에 올린다. (Dirty Checking 대상으로 등록) | ||
| AbtestVariable variable = abtestVariableRepository.getById(abtestVariableCount.getVariableId()); | ||
| variable.addCount(abtestVariableCount.getCount()); // RDB count update | ||
| abtestVariableCount.resetCount(); // Redis reset | ||
| } | ||
| // reset된 값들을 반영 | ||
| abtestVariableCountRedisRepository.saveAll(cachedCount); | ||
| // -- syncCacheCount 끝 |
There was a problem hiding this comment.
A
이 로직(syncCacheCount)이 사용자 편입부분에서 직접 포함되면 안되는데 제가 왜 넣었는지 잘 모르겠네요..
이렇게 사용하면 Redis를 활용한 이유가 없을텐데... 😥
+) syncCacheCount는 다른 메서드에서도 사용중인 로직인데 여기에 원본을 복사해 넣은 이유는 리뷰 간 가독성 향상을 위해서인가요..??
| TODO : 해당 부분 설명 필요 | ||
| 조건문 진입 시점에서 유저아이디가 Null이 아님을 보장하고있는데 이 로직이 존재하는 이유는? | ||
| 기존 | ||
| if (userId != null) { | ||
| createDeviceIfNotExists(userId, userAgentInfo, accessHistory, abtest); | ||
| } |
There was a problem hiding this comment.
userId != null 조건이 기입된 이유는 기존 로직 상 조건문 진입 시점에서 유저아이디가 Null이 아님을 보장하지 못하고 있기 때문입니다.
기존 로직 코드는 아래와 같습니다.
private boolean isAssignedUser(Abtest abtest, Integer accessHistoryId, Integer userId) {
AccessHistory accessHistory = accessHistoryRepository.getById(accessHistoryId);
if (userId != null && accessHistory.getDevice() != null
&& !Objects.equals(accessHistory.getDevice().getUser().getId(), userId)) {
return false;
}
return abtest.getAbtestVariables().stream()
.anyMatch(abtestVariable -> accessHistory.hasVariable(abtestVariable.getId()));
}여기서 false가 반환되는 경우 기존에 편입되지 않은 사용자로 판단하고 이후 로직을 진행합니다.
수정해주신 로직에서는 첫 조건문으로 전체 로직을 감싸주셨지만 첫 조건문이 만족하지 못하더라도(비로그인 사용자라도) false가 반환되는 경우(accessHistory가 대상 abtest에 아직 속하지 않은 경우)는 존재합니다.
이 케이스에서는 비로그인 사용자도 여기까지 넘어올 수 있기 때문에 한번 더 검증해주었던 걸로 기억하고 있습니다.
+) createDevice를 여기서 진행하는 이유는 기존에는 비로그인으로 사용중이었던 기기가 로그인으로 바뀔 경우 기기 정보를 기록하기 위해서였던 걸로 기억하고 있습니다.
| // TODO: 설명 필요 | ||
| // if (userId != null) { | ||
| // createDeviceIfNotExists(userId, userAgentInfo, accessHistory, abtest); | ||
| // } |
There was a problem hiding this comment.
여기(assignOrGetVariable)서도 이 메서드를 호출하는 이유는 getMyVariable을 호출하지 않는 경우(새로 편입되는 accessHistory의 경우)에도 디바이스 생성 로직이 필요하기 때문입니다.
🔥 연관 이슈
🚀 작업 내용
💬 리뷰 중점사항
abtest의 로직 흐름이 이해가 가지 않아 중복되지 않는데 메소드로 분리되어있는 내용들을 다 하나로 합쳐놨습니다.
로직의 흐름에 대해 설명해주실 수 있나요?