FIX: Change syncronization tool in getVersion().

[brido4125]

Motivation

기존의 getVersion에서 동시성 이슈 해결은 synchronized 블럭을 통해 진행한다.
이는 쓰레드의 blocking을 야기해 성능의 문제가 존재한다.
또한 현재 synchronized 블럭의 인자인 VERSION은
synchronized 블럭 내에서 참조가 변경될 수 있다.
즉, threadA가 lock을 걸고 내부 로직에서 참조를 변경하면
threadB가 lock을 거는 참조 대상이 달라져
동기화가 제대로 이루어지지 않을 가능성이 존재한다.

변경지점

이러한 문제를 AtomicReference를 통해 해결합니다.
기존과 동일하게 버전 파싱 중 예외 발생 시는 "NONE"을 대입합니다.

[jhpark816]

@brido4125

• 응용 처음 구동하는 시점에 호출하는 메소드이므로, synchronized block 이어도 됩니다.
• VERSION 참조가 변경되어 동기화 문제가 있다는 것이 구체적으로 무엇인가요?
  기존 구현에 문제가 있었다는 것인가요?

[brido4125]

VERSION 참조가 변경되어 동기화 문제가 있다는 것이 구체적으로 무엇인가요?
기존 구현에 문제가 있었다는 것인가요?

기존 로직은 아래와 같습니다.

synchronized (VERSION) {
    // 내부에서 VERSION의 참조값 변경
}

현재 로직에서 Thread A가 VERSION을 처음으로 변경하려 할 때는
"INIT" 이라는 값을 가지는 참조에 대해 Lock을 걸고 내부 로직을 수행합니다.
로직을 수행하며 내부에서 VERSION에 새로운 참조를 할당합니다.
이 때 스레드 B가 synchronized 블럭에 진입 시도를 하게 될 경우
참조가 "INIT"에 대한 참조가 아니기 때문에 진입 가능합니다.
동일한 변수 명이지만 시점에 따라 갖고 있는 참조가 달라집니다.
결론적으로 synchronized 블럭이 정상적으로 동작하지 않을 수도 있습니다.

물론 스레드 B의 수행 로직은 블럭 내부의
if (VERSION.equals("INIT")) 조건문에 의해
걸러지기 때문에 로직 상 문제는 없다고 볼 수 있습니다.

다만 Atomic을 쓰면 기존에 비해
조건문을 Depth를 줄일 수 있어
구현이 좀 더 간결해져 이해하기 수월하다고 생각됩니다.

[uhm0311]

다른 곳에서 VERSION의 참조를 바꾸지 않으므로 final로 선언합시다.

[uhm0311]

2회의 get() 사이에 값이 바뀔 수 있으므로 지역변수에 VERSION.get()을 담아두고 if문과 return문에서 사용합시다.

[jhpark816]

@brido4125 @uhm0311

• synchronized block 사용 시에 동시성 문제가 전혀 없습니다.
  without locking으로 먼저 확인하고 나서 (locking 줄이기 위해),
  with locking으로 정확히 확인하는 방식은 일반적인 패턴입니다.
• PR의 atomic value 사용 방식에서는
  version 획득하는 로직이 여러번 수행될 수 있습니다.

따라서, 이 경우는 synchronized block 방식이 낫다고 봅니다.
검토해 보세요.

[uhm0311]

synchronized (VERSION) => synchronized ("INIT")
이렇게만 바꿔도 된다고 봅니다.

[jhpark816]

리뷰 완료

[jhpark816]

동시 접근을 순서 접근으로 하려는 대상이 VERSION 객체이며, "INIT" 값은 아님.
따라서, 기존 코드가 논리적으로 이해하기 나음.

[uhm0311]

기존 코드에서 2개의 Thread가 getVersion() 메소드를 호출했을 때 다음과 같이 동작합니다.

• 1번 Thread가 VERSION.equals("INIT") == true이므로 return 하지 않은 상태
• 2번 Thread로 Context Switch. 아직은 VERSION.equals("INIT") == true이므로 return 하지 않은 상태
• 1번 Thread로 Context Switch. 1번 Thread는 VERSION 객체의 Lock을 획득합니다. VERSION 객체의 주소 값이 0xAAAAAA라고 가정합니다. 1번 Thread가 0xAAAAAA의 주소로 잡은 Lock을 획득한 채로 VERSION 객체의 값 변경. 변경된 VERSION 값의 주소 값을 0xBBBBBB라고 가정합니다.
• 2번 Thread로 Context Switch. 2번 Thread는 변경된 VERSION 객체의 Lock, 즉 0xBBBBBB의 주소로 Lock을 획득합니다. 이 상태로, 변경해도 그대로겠지만, 다시 VERSION 객체의 값 변경

최대 2개의 Thread에 의해 VERSION 설정 로직이 돌아갈 수 있습니다.

[brido4125]

@uhm0311

2번 Thread로 Context Switch. 2번 Thread는 변경된 VERSION 객체의 Lock, 즉 0xBBBBBB의 주소로 Lock을 획득합니다. 이 상태로, 변경해도 그대로겠지만, 다시 VERSION 객체의 값 변경

0xBBBBBB의 참조를 가진다는 것이
이미 VERSION 변수의 값은 "INIT"이 아니기 때문에
version 정보를 얻어오는 로직은 수행되지 않는게 아닐까요?

2번 쓰레드의 경우 critical section에 진입해서
if (VERSION.equals("INIT"))를 수행하고
조건을 만족하지 않아 그대로 VERSION을 리턴합니다.

---

"INIT" 이라는 값 자체를 아래와 같이 전역 상수화 시키면
INIT이라는 인스턴스를 통해 critical section에
접근 하려는 스레드들을 제어하려는
의미가 잘 드러난다고 생각합니다.

  private static final String INIT = "INIT";
  private static String VERSION = INIT;

[jhpark816]

@uhm0311 @brido4125
synchronized (VERSION) 과 같이 특정 변수 명을 주는 경우의 내부 동작에 대해 잘 몰랐던 것 같습니다.
지적한 문제를 해결하기 위해,

• synchronized (this)와 같이 this 변수를 사용하거나
• 별도의 versionLock = new Object(); 객체를 만들어 synchronize하는 것이 좋겠습니다.

[brido4125]

아래와 같이 this를 사용하는 경우는
getVersion 메서드가 static이라서 불가합니다.

synchronized (this) {
    // ...
}

static 객체를 하나 만들어서 인수로 사용하도록 하겠습니다.

[jhpark816]

@uhm0311 @oliviarla
새로운 구현 방식을 확인 바랍니다.