FIX: Concurrency problem in locator allNodes.

[brido4125]

이슈

https://github.com/jam2in/arcus-works/issues/490#issuecomment-2199206597
위 코멘트 말고도 Braodcast 연산 시에 getAll()을 호출하는데
read는 worker-thread 로 write는 IO 스레드에 의해서
ConcurrentModificationException이 발생할 수도 있습니다.

왜냐하면 기존 구현은 리턴되는 Collections.unmodifiableCollection이
allNodes 참조와 연결되어있기 때문입니다.

반환 값에 새로운 참조를 넣어서 기존 allNodes 참조와의
연결을 끊어주면 동시성 문제가 발생하지 않습니다.

다른 대안으로는 new CopyOnWriteArrayList<>(allNodes);와 같은 타입을 리턴할 수도 있습니다.
다만 alterNodes와의 구현 통일성 때문에 현재 구현을 채택했습니다.

참고로 alterNodes는 위와 같이 서로 다른 스레드가
각각 동시에 read/write 하는 로직이 존재하지 않습니다.

[jhpark816]

리뷰 완료

[jhpark816]

@oliviarla 리뷰 바랍니다.

[brido4125]

@jhpark816

오프라인에서 말씀하신 사항에 대한 답변입니다.
ketamaNodes를 read하여 key에 해당하는 노드를 선정할 때
update() 로직에서 사용되는 동일한 lock 객체를 사용합니다.

MemcachedNode getNodeForKey(long hash) {
    lock.lock();
    try {
      if (ketamaNodes.isEmpty()) {
        return null;
      }
      Map.Entry<Long, SortedSet<MemcachedNode>> entry = ketamaNodes.ceilingEntry(hash);
      if (entry == null) {
        entry = ketamaNodes.firstEntry();
      }
      return entry.getValue().first();
    } finally {
      lock.unlock();
    }
  }

[jhpark816]

@oliviarla 한번 더 점검하고, 의견주세요.

[oliviarla]

@jhpark816
제가 확인한 바로는 allNodes를 사용하는 메서드는 IO스레드에서만 호출되고, update 메서드 자체도 IO 스레드에서만 호출되기 때문에 문제가 없을 것 같습니다.

[brido4125]

@jhpark816 @oliviarla

어떤 관점에서 점검을 지시 하셨는지는 모르겠는데,
단순히 Broadcast 연산만 하더라도 응용의 스레드가
allNodes를 read 해갈 수 있지 않나요?

추가적으로 locator를 리팩토링하는 설계에 있어
allNodes도 변경이 생길 것 같아 본 PR은 draft로 돌려놓겠습니다.

[jhpark816]

@brido4125 rebase 해 주시죠.

[brido4125]

@jhpark816

본 PR은 현재 진행중인 이슈(Locator 공유)에 의해서 변경될 가능성이 굉장히 높습니다.
(allNodes가 tcp 커넥션의 리스트라서 현재 구현처럼 Locator 클래스 내에 존재하지 않을 확률이 높음)

그래서 close를 시켜놓고 보류하려고 합니다.

[jhpark816]

@brido4125
draft => open 해 주세요.

@uhm0311 @oliviarla
현재 PR 그대로 반영하면 될 것 같은데요.
한번 확인 바랍니다.

[uhm0311]

충돌 난거만 해결하면 될 것 같습니다.