잘못된 synchronized 사용 사례

, ,

SCENARIO

하루에 10명만 받아야하는 신청프로그램 개발건이 들어왔다.
초보 개발자 김삽질씨는 나름 열심히 고민해서 프로그래밍했다
“신청자가 10명이 넘으면 안되니까 신청자가 몇명인지 조회하고 10명 이상이면 신청이 안되게 해야겠다~”

1
2
3
4
5
6
7
8
9
10
11
12
13
14
public class ApplicantManager {
    private static final List<Integer> applicantList = new LinkedList<>();
    private static final int APPLICANT_LIMIT = 10;
	
    public void applicate(int id) {
        if(applicantList.size() < APPLICANT_LIMIT) {
            applicantList.add(id);
        }
    }
	
    public static List<Integer> getApplicantList() {
        return applicantList;
    }
}

개발이 끝나고 신청 당일날 고객한테 전화가 왔다.

“총 신청자가 12명인대요?”

헬게이트가 열렸다 ㄷㄷㄷ

망함

김삽질씨는 왜 초과신청이 되었는지 알수가 없어 선배 개발자에게 물어보았다. 돌아온 대답은..

“동시에 신청 못하게 동기화를 시켜줘야돼~”

선배말을 듣고 동기화에 대해 알아본 후 Java엔 synchronized라는게 있는걸 알게됐다.
그리고 야심차게 코드를 수정한다.

1
2
3
4
5
6
7
8
public class ApplicantManager {
    ....
    public synchronized void applicate(int id) {
        if(applicantList.size() < APPLICANT_LIMIT) {
            applicantList.add(id);
        }
    }
}

그리고 다음날 총 신청자 수는…



11명이었다…

도망

뭐가 잘못된걸까? 분명 applicate() 메서드를 동기화해서 동시접근을 막았는데..
김삽질씨는 synchronized에 대해 더 자세히 알아보았고 결국 문제점을 알게되었다.
다음은 ApplicantManager를 호출하는 부분이다.

1
2
ApplicantManager mananger = new ApplicantManager();
mananger.applicate(id);

호출할 때마다 ApplicantManager의 새로운 인스턴스를 만들고 있다.
그런데 메서드 synchronized는 lock의 기준을 인스턴스로 한다는 사실을 몰랐던 것이다.
서로 다른 인스턴스간에는 동기화가 될리가 없었다. 중요한 깨닮음을 얻고 다시 코드를 수정했다.
방법은 세가지다.

  • ApplicantManager를 싱글톤으로 만드는 방법
  • 메서드에 static synchronized 거는 방법
  • synchronized 블록을 이용하는 방법

synchronized 블록을 선택했다.

1
2
3
4
5
6
7
8
9
10
public class ApplicantManager {
    ....
    public void applicate(int id) {
        synchronized(ApplicantManager.class) {
            if(applicantList.size() < APPLICANT_LIMIT) {
                applicantList.add(id);
            }
        }
    }
}

그리고 다음날부터는 10명을 넘지않았다는 훈훈한(?) 이야기다 ^^

이젠 정말로 Thread-safe 해진걸까?
불행히도 아직 잠재적인 문제가 남아있다.
ApplicantManager의 getApplicantList() 메서드가 public이라서 applicantList의 값이 외부에서 변경될 소지가 있다. applicantList를 외부로부터 감추자!

또 하나의 문제점은 synchronized의 인자를 ApplicantManager.class로 하고 있다는 점이다.
만약에 관리자가 임의로 신청을 입력하는 기능이 추가되어 ApplicantAdminManager를 만들고
신청자를 추가하는 메서드를 만든다면 또 초과 신청이 발생할 수 있다.
synchronized(applicantList)로 바꿔주자!

TEST

위의 시나리오를 테스트해보자. 동시요청을 가정하기 위해서 3개의 쓰레드를 돌려 10번씩 신청했다.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
public class ThreadTest {
    public static void main(String[] args) {
		
        Runnable applicateTask = () -> {
            ApplicantManager mananger = new ApplicantManager();
            for(int i=0; i<10; i++) {
                mananger.applicate(i);
            }
        };
		
        Thread t1 = new Thread(applicateTask);
        Thread t2 = new Thread(applicateTask);
        Thread t3 = new Thread(applicateTask);
		
        t1.start();
        t2.start();
        t3.start();
        try {
            t1.join(5);
            t2.join(5);
            t3.join(5);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
		
        System.out.println("총 신청자 인원 : " + ApplicantManager.getApplicantList().size());
    }
}

4가지 상황별로 10번씩 실행한 결과는 다음과 같다.

not use synchronized method synchronized synchronized block Singleton Pattern
총 신청 인원 10 12 10 10
총 신청 인원 12 12 10 10
총 신청 인원 11 12 10 10
총 신청 인원 10 12 10 10
총 신청 인원 12 11 10 10
총 신청 인원 12 12 10 10
총 신청 인원 12 11 10 10
총 신청 인원 12 12 10 10
총 신청 인원 12 10 10 10
총 신청 인원 12 12 10 10

synchronized를 안쓰거나 잘못사용한 경우 대부분 10명이 넘게 신청되었다.

Conclusion

위 시나리오에서 Thread-safe하지 않은 근본원인은 static 필드를 사용하기 때문이다.
Servlet기반의 Java웹서버들은 기본적으로 사용자의 요청을 병렬적으로 처리하지만
대부분 1 request 당 1 thread이고 독립적인 context에서 쓰레드가 실행되기 때문에 문제될게없다.
하지만 static이나 상태를 가지고 있는 singleton 객체는 멀티스레드에서 공유될 수 있기 때문에 Thread-safe한지 고려해야한다.
Thread-safe한 변수만 두면 된다는 단순한 규칙만으로도 많은 문제를 예방할 수 있고,
java.util.concurrent 패키지의 객체사용도 고려해보면 좋겠다