생에 첫 오픈소스 컨트리뷰트 도전기 (2)

https://seongonion.tistory.com/145

생애 첫 오픈소스 컨트리뷰트 도전기 (1)

위 글과 이어지는 내용입니다.

 

테스트 코드를 추가하고 얼마 후, 리뷰를 해주던 jmehrens가 갑자기 살짝 갸우뚱한 얘기를 했다.

대략적으로 해석해보자면, 루트 스트림이 닫히면 newStream() 으로 생성된 하위 스트림은 모두 닫히도록 문서화가 되어있고, 이를 기반으로 보면 내가 마주친 문제는 해당 클래스의 "의도된 행동"이라는 것이다.

 

따라서 해당 이슈는 won't fix로 닫힐 수 있다는 것.

 

내가 뭔가 핀트를 잘못 이해하고 있나 싶어서 수십 번 읽어봤지만 역시나 이해가 잘 안됐다.

 

내가 제시하는 이슈는 "루트 스트림이 닫히면 하위 스트림까지 닫혀요. 루트 스트림 닫혀도 하위 스트림은 살려주세요." 가 아니라

 

"루트 스트림이 의도치 않게 G.C에 의해서 닫혀요. 그래서 하위 스트림까지 못쓰게 돼요" 였다.

 

다시 말해, PR을 통해 제시하고자 하는 내용은 "루트 스트림이 G.C에 의해 예상치 못하게 수거되지 못하도록 하자." 였다.

해당 내용 또한 최대한 오해없이 명확히 알아들을 수 있도록, 그리고 내가 뭔가 잘못 이해한게 있다면 지적해줄 수 있도록 명명백백하게 내 의도를 설명하려고 노력했다.

 

또한, 해당 문제를 해결한다고 하더라도 내가 제안한 방식처럼 루트 스트림을 고정해놓는 방식보다는 참조 카운트 방식을 사용하고 싶어하는 것처럼 보여서, 그것도 내가 처음에 고려한 방식이긴 하지만 그러려면 finalize() 메서드가 제거되어야 가능하다는 언급도 더했다.

 

내 의도를 조금 더 자세히 설명하기 위해, jmehrens가 요청한 실제 환경에서 발생한 문제를 예시코드와 함께 설명했다.

public List<InputStream> extract() {

    MimeMessage message = new MimeMessage(session, new SharedFileInputStream(getFile()));

    List<InputStream> results = new ArrayList<>();
        
    foreach (mimePart of message) { 
        if (...) {
            InputStream extractedInputStream = mimePart.getDataHandler().getDataSource().getInputStream();
            results.add(extractedInputStream);
        }
    }
    return results;
}

public List<Integer> getSize() {
    List<InputStream> extracted = extract();

    return extracted.stream()
        .map(is -> is.available())
        .collect(Collectors.toList());
}

 

먼저 extract()에서 SharedFileInputStream을 통해 파일을 불러와 MimeMessage를 생성 후, 해당 MimeMessage를 구성하는 MimePart의  특정 InputStream을 뽑아 리스트 형태로 리턴해준다.

 

여기서 주목할 것은, new MimeMessage(session, new SharedFileInputStream()) 호출 시 MimeMessage 생성자는 인자로 받은 스트림이 SharedInputStream인 경우 해당 스트림을 그대로 사용하지 않고 newStream()을 호출해 생성한 하위 스트림을 참조한다.

 

MimeMessage의 생성자에서 SharedInputStream을 처리하는 방식

 

즉, extract()에서 MimeMessage를 생성하며 인자로 넣었던 루트 스트림인 SharedFileInputStream은 더 이상 참조가 되지 않는다.

 

이후, getSize()에서 가져온 리스트의 스트림의 메서드를 호출하게 되면  IOException: Stream Closed 를 뱉게된다.

 

extract() 메서드 호출 후, 더 이상 참조되지 않는 루트 스트림이 GC에 의해 제거되어 더 이상 루트 스트림에 의해 만들어진 하위 스트림에 접근할 수 없게 되는 것이다.

해당 케이스를 보여주고나서는 won't fix가 should fix로 바뀌었다.

 

이후엔 how to fix에 대한 이야기를 계속해서 나눴다.

 

처음엔 finalizer를 제거하고 SharedFile에 Cleaner API를 사용하는 것을 선호한다고 하였으나, 해당 방식을 적용하기 전에 다른 두 가지 방식을 제안했다.

 

첫 번째는 close()에 synchronize 키워드를 추가하는 것이었고, 두 번째는  java.lang.ref.Reference.reachabilityFence 을 newStream()에 추가하는 것이었다.

reachabilityFence는 처음 보는거라 공식 문서를 읽어봤는데, 특정 객체를 strongly reachable하게 만들어서 GC에 의해 수거되는 것을 막을 수 있다 정도로 이해했다. (아래 참조)

Ensures that the object referenced by the given reference remains strongly reachable, regardless of any prior actions of the program that might otherwise cause the object to become unreachable; thus, the referenced object is not reclaimable by garbage collection at least until after the invocation of this method. Invocation of this method does not itself initiate garbage collection or finalization.

(https://docs.oracle.com/javase%2F9%2Fdocs%2Fapi%2F%2F/java/lang/ref/Reference.html#reachabilityFence-java.lang.Object-)

 

메서드명 또한 꽤나 직관적으로, 접근가능한 객체들을 모아놓는 reachability 라는 펜스에 객체를 집어넣는 느낌이었다.

 

newStream()에 집어넣는 이유 또한 하위 스트림을 생성할 때 상위 스트림이 GC에 의해 잡히지 않도록 하기 위함이었다.

 

두 번째로, close() 에 synchronized 키워드를 추가하라는 제안이었는데, 이건 사실 잘 이해가 안됐다.

 

내가 알고 있는 synchronized는 멀티 스레드 환경에서 여러 개의 스레드가 특정 메서드에 동시에 접근하지 못하도록 배타락을 걸기 위한 용도였다.

 

그래도 우선 제안했던 2 가지 방식을 각기, 그리고 모두 적용해보고 테스트 해보았고 이를 정리해서 답글을 달았다. (해당 상황에서 테케는 모두 실패했다)

 

또한, 사용이유에 대해 잘 이해가 안갔던 synchronized 키워드에 대해서도 질문을 남겼다.

그리고 이에 대해 꽤나 친절하게 답변을 해주셨다.

테스트가 실패했던 이유는 루트 스트림이 닫힐 때 모든 하위 스트림이 닫힌다는 컨셉 자체가 유지되기 때문이라고 했다.

 

즉, newStream()에 reachabilityFence를 추가해 하위 스트림이 생성될 때 상위 스트림이 G.C에 의해 제거되는 것은 막았으나, 그렇다고 이것이 루트 스트림을 G.C로 부터 보호한 것은 아니기 때문이었다.

 

따라서, 제안했던 방식이 유효하려면 기존의 루트 스트림이라는 개념 자체도 없애야하고, 이에 맞게 forceClose()와 같은 메서드도 함께 없애야한다는 의미였다.

 

잘 이해가 되지 않았던 synchronized 키워드 역시 스레드가 해당 메서드에 대한 락을 획득하기 때문에 메서드 실행 중에는 GC가 해당 객체를 수거할 수 없도록 한다는 의미였다.

 

SharedFileInputStream의 경우, newStream()이 synchronized로 선언되어 있기 때문에 close()도 synchronized로 선언되어야 newStream() 호출 중 close()가 호출되지 않도록 막을 수 있다.

 

흠.. 근데 G.C가 언제 실행될 수 있는지 예측할 수 있는 것도 아니고 newStream() 호출 동안 close() 가 호출되지 않도록 막는다고 문제가 다 해결이 되는게 맞나??

 

하여간 이건 나중에 조금 더 자세히 알아보자.

실제로 루트 스트림에 대한 개념을 걷어내고 나서는 jmehrens가 제안한 방식의 코드가 테스트를 통과했다..!

위 두 방법 이전에 처음으로 제안했던 finalize() 제거 + SharedFile에 Cleaner API를 사용하는 방식에 대해서도 좋은 방법인 것 같다고 코멘트를 달고 내가 작업할까요? 라고 물어봤더니, 해당 문제에 대해서 내부적으로 합의를 해보겠다고 이야기했다.

 

나는 OK라고 답했다.

 

그리고 약 일주일 후...

검토내용 및 추후 작업 내용에 대해서 공유해줬다.

요약하자면 다음과 같았다.

 

1. close()는 author의 의도로 인해 synchronized로 감싸지지 않은 것 같다. 이는 추후 다른 작업에서 처리할테니 우선 reachabilityFence를 사용해달라.

 

2. newStream()에도 reachabilityFence를 사용해달라. 사실 해당 메서드는 synchronized 이기 때문에 기능적으론 불필요하지만 상위 스트림이 살아있어야한다는 의도를 보여주기 위함이다.

 

3. 루트 스트림에 대한 개념을 제거해라. 따라서 forceClose()도 함께 제거해줘야한다. 추가로 이에 대한 Docs도 삭제해달라.

 

4. finalizer 역시 모두 synchronized 로 감싸달라. Cleaner API 사용은 현재 PR에서 너무 많은 작업을 요하므로 우선 사용하지 말아달라.

수정 완료 후 테스트 케이스 역시 모두 통과했다.

 

결과적으로 내가 처음에 제안한 코드는 모두 없어졌지만.. 🥲 그래도 꽤 긴 시간 글로만 소통하며 문제를 해결해가는 과정이 참 좋았던 것 같다.

 

이젠 별 탈 없이 무사히 머지되길..