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

[oneseo] 성적 계산 싱글톤 필드 동시성 문제 ThreadLocal 사용으로 개선 #229

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

tlsgmltjd
Copy link
Member

개요

성적 계산 클래스(CalculateGradeService) 싱글톤 클래스 필드 동시성 문제를 ThreadLocal을 사용하여 개선하였습니다.

본문

성적 계산시 개별학기 환산점은 계산 메서드 여러 곳에서 사용되기 때문에 필드로 빼서 구현하게 되었습니다.
하지만 이러한 구조로 인해 싱글톤으로 관리되는 빈 특성상 여러 스레드가 필드를 공유하게 되고, 동시성 문제가 발생하였습니다.
#187 이러한 문제는 계산 메서드 시작 전에 필드를 초기화 하는 방식으로 해결한 작업이 반영되었습니다.

하지만 이러한 대처는 비슷한 방식의 동시성 문제를 야기할 수 있을 것이라 생각되어 불안전한 방식입니다.

그래서 현재 구조를 크게 변경하지 않으면서 안정성을 보장할 수 있는 방법인 ThreadLocal을 도입하여 문제를 개선해보았습니다.
ThreadLocal은 여러 스레드가 공유하는 필드를 각각의 스레드의 저장공간에 저장하여 해당 필드를 ThreadSafe하게 보장해주는 기능입니다.

기존 코드 구조를 크게 변경하지 않고 적용할 수 있어서 괜찮은 방식이라 생각하였습니다.

@tlsgmltjd tlsgmltjd requested a review from jangwooooo January 10, 2025 04:31
@tlsgmltjd tlsgmltjd self-assigned this Jan 10, 2025
@YangSiJun528
Copy link
Member

YangSiJun528 commented Jan 10, 2025

싱글톤 객체가 (특히 변경이 많은) 상태를 가지는 건 좋지 않은 구현이에요. 지금 겪으신 것처럼 동시성 문제가 발생할 수 있습니다.

저는 점수 데이터가 execute() 메서드와 동일한 생명주기를 가지는게 좋다고 생각하는데, scoreN_N 변수를 클래스 레벨로 만든 이유가 있나요?

@YangSiJun528
Copy link
Member

YangSiJun528 commented Jan 10, 2025

여러 곳에서 사용되는 데이터라서 클래스 변수로 사용했다면,
메서드 내부에서 DTO를 만드는게 동시성 문제에도 안전하고 데이터 흐름도 직관적이라고 생각해요.

@tlsgmltjd
Copy link
Member Author

답변 감사드립니다 ! 처음 성적 계산 클래스를 작성할 때 여러 private 메서드에서 score 값을 변경하고 조회하는 구조가 되어서 클래스 필드로 빼는 구조로 작성하게 되었습니다.

현재 개선하게된 내용은 현재 구조에서 코드 수정을 가장 적게 하면서 개선할 수 있는 부분이라 생각하여 ThreadLocal을 사용하여 개선하게 되었습니다.

하지만 선배님 말씀처럼 이러한 객체가 상태를 가지는 것은 좋지 않은 구현이라고 생각합니다. 제안해주신 방법처럼 DTO를 만들어서 개선하는 것이 최선의 방법이라고 생각합니다.

이를 반영하여 추가로 개선작업을 진행해보도록 하겠습니다. 감사합니다.

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.

2 participants