Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Notification 생성 시 새로 save하는 경우 트랜잭션 분리한다 #193

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

melonturtle
Copy link
Contributor

What is this PR? 🔍

  • Notification 생성 시 데드락이 발생할 수 있는 여지가 있습니다.
    • topic의 status, 투표수, 좋아요수를 수정하고 event를 publish하면 notification이 생성되는데, 이 때 모두 db 차원에서는 같은 트랜잭션에서 실행됩니다.
    • 이로 인해 notification 생성시 topic에 대한 S락을 갖고 있는 상태에서 notification insert 후에 나중에 topic에 대한 X락을 서로 얻으려고 하면서 문제가 발생할 여지가 있습니다.
  • 그래서 새로운 트랜잭션으로 분리했습니다.
    • 다른 방법도 있긴 한데 제 생각엔 이 방법이 가장 나은 것 같아서 이렇게 수정했습니다. 블로그 글에 더 자세한 내용은 적어놨습니다. 다른 더 좋은 방법 있으시면 알려주세요!
  • 테스트는 따로 작성하지 않았고 실행해서 트랜잭션이 잘 분리되고 락이 해제되는 걸 확인했습니다.

🛠️ Issue

@melonturtle melonturtle requested a review from 60jong March 13, 2024 12:51
@melonturtle
Copy link
Contributor Author

아 이거 테스트 실패하는건, 다른 트랜잭션으로 분리하면 테스트에서 생성해준 Member와 Topic 정보가 다른 트랜잭션엔 반영되지 않는 것 때문이라 먼저 db에 반영되도록 수정하면 통과할 것 같습니다. 일단 무시하고 봐주세요!

@melonturtle melonturtle force-pushed the concurrency/notification branch 3 times, most recently from f27ce64 to 6e2b4de Compare March 13, 2024 15:38
@melonturtle melonturtle force-pushed the concurrency/notification branch from 6e2b4de to a3f573e Compare March 13, 2024 15:45
@melonturtle
Copy link
Contributor Author

melonturtle commented Mar 13, 2024

테스트 수정 완료했습니다.

테스트 발생 이유가 테스트에선 Member, Topic 등 생성 후에 NotificationService도 같은 트랜잭션 안이므로 엔티티들 생성 정보를 볼 수 있었는데, NotificationService가 새로운 트랜잭션 안에서 수행되므로 다른 트랜잭션의 커밋되지 않은 정보는 볼 수 없기 때문입니다.

그래서 TestTransaction을 이용해 먼저 커밋되게 해줬습니다.

TestTransaction을 직접 사용할 때는 롤백이 되지 않기 때문에 @AfterEach에서 테이블 데이터를 직접 지워주게 했습니다.

TestTransaction 말고 TransactionTemplate을 이용하는 방법도 있는데.. TestTransaction이 더 간단할 것 같아 이 방법으로 구현했는데 딱히 그러진 않은 것 같네요.. 아마 다른 동시성 테스트는 TransactionTemplate을 이용할 것 같습니다! 테스트 관련된 건 다른 서브 이슈에서 또 구현할 거라 자세한 건 거기서 또 언급할 것 같아요~!

@melonturtle
Copy link
Contributor Author

melonturtle commented Mar 14, 2024

앗 근데 생각해보니까.. 새로운 트랜잭션으로 나누는 방법은 스케쥴러에서 투표 끝나는 거 알림 생성하는 상황에서만 생각했던 건데, 다른 상황에서도 적절할진 모르겠어요.

EventListener가 지금 비동기가 아니니까, NotificationService의 트랜잭션이 끝날 때까지 기다릴텐데 데이터베이스와 기본 커넥션 개수가 10개거든요. 그래서 요청 6개를 동시에 처리하고 있을 때, NotificationService가 내부 트랜잭션을 하나씩 더 사용하면, 내부 트랜잭션을 위한 커넥션을 기다려야 하는데 외부 트랜잭션이 다 갖고 있어서 커넥션을 얻지 못하고, 또 외부 트랜잭션은 내부 트랜잭션이 안끝나니까 계속 기다리는 상황이 발생할 것 같아요.

개인적으로는 EventListener가 비동기가 되는 편이 나을 것 같은데, 처음에 EventListener를 추가한 이유도 알림 로직과 일반 로직을 의존성 없게 분리하자는 목적이었는데 사실 지금은 코드로는 의존성이 안보이지만 NotificationService의 트랜잭션에서 문제가 생기면 다 롤백되어서 암묵적으로는 의존성이 있다고 생각하거든요. 근데 이거까지 지금하기엔 더 급한게 많으니까 나머지 부분은 좀 더 생각해보고 우선 스케쥴러에서 호출되는 쪽에만 남겨놓을게요 ... 😂

@melonturtle melonturtle merged commit 12265ce into main Mar 27, 2024
1 check passed
@melonturtle melonturtle deleted the concurrency/notification branch March 27, 2024 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notification 관련 외래키로 인한 데드락 가능성
1 participant