だから、あれほどFiles#lines(Path)を使うときはtry-with-resourcesでちゃんと包めといったのに… #jjug #ccc_f2

以前、こういう記事を書きました。

mike-neck.hatenadiary.com

僕は、世間的にあまり知られていない人間なので、この記事があまり伝わっていなかったのでしょう。ちょっと恐れていたことがありました。

JJUG CCC 2015 SpringのセッションF2で、このような発表がありました。

ほんとうに便利だった業務で使えるJava SE8新機能（JJUG CCC 2015 Spring） from Yuuki Fukuda

www.slideshare.net

これの33ページのコードにはリソース解放漏れがあります。

FIles.lines(path).lines().forEach(System.out::println);

このバグの可能性のあるコードがJJUG CCCというかなり規模の大きくなってきた大舞台でサンプルコードとして紹介されてしまいました。規模の大きいイベントでの誤った事例の紹介を後々で回収するしていくのはかなり面倒な作業です。なので、できるだけ早く誤りを指摘しようと思い、急いでこの記事を改定ます。

---

【2015/04/13 7:59 追記】 一応39ページに次のように書かれています。これは僕の見落としでした。

StreamはAutoClosableを実装しているからtry-with-resources文のリソースとして利用可能

---

さて、件のFiles#lines(Path)というメソッドですが、次のような実装になっています。

public static Stream<String> lines(Path path, Charset cs) throws IOException {
    BufferedReader br = Files.newBufferedReader(path, cs);
    try {
        return br.lines().onClose(asUncheckedRunnable(br));
    } catch (Error|RuntimeException e) {
        try {
            br.close();
        } catch (IOException ex) {
            try {
                e.addSuppressed(ex);
            } catch (Throwable ignore) {}
        }
        throw e;
    }
}

これはBufferedReader#lines()を呼び出して、Stream<String>を取得して、返すメソッドになっています。その際に、onCloseにてasUncheckedRunnable(BufferedReader)が呼び出されており、次のように実装されています。

private static Runnable asUncheckedRunnable(Closeable c) {
    return () -> {
        try {
            c.close();
        } catch (IOException e) {
            throw new UncheckedIOException(e);
        }
    };
}

つまり、onCloseで指定されたRunnableではBufferedReader#closeが呼ばれるようになっています。このonClosedで渡されるRunnableが評価・実行されるのはStream.closeが実行されたタイミングです。

ただ、しかし、Stream<T>の終端処理ではcloseが呼ばれることはありません。

実際にcloseが呼ばれていないことを確かめるために、BufferedReader#lines()メソッドの部分を流用した次のようなテストコードを書いてみました。

private final List<String> list = Arrays.asList("a", "b", "c", "d");

private static class OnClose implements Runnable {
    private boolean called = false;
    @Override
    public void run() {
        called = true;
    }
    public boolean isCalled() {
        return called;
    }
}

private Stream<String> getStream(final OnClose onClose) {
    // BufferedReaderのlinesの実装の真似
    Stream<String> stream = StreamSupport.stream(
            Spliterators.spliteratorUnknownSize(
                    list.iterator(),
                    Spliterator.ORDERED | Spliterator.NONNULL), false);
    stream.onClose(onClose);
    return stream;
}

// try-with-resourcesを用いないコードでcloseが呼び出されないことを確認するテスト
@Test
public void withoutTryWithResources() {
    OnClose onClose = new OnClose();
    Stream<String> stream = getStream(onClose);
    stream.forEach(System.out::print);
    // false なら closeメソッドが呼ばれていない
    assertThat(onClose.isCalled(), is(false));
}

// try-with-resourcesを用いたコードではcloseが呼び出されることを確認するテスト
@Test
public void withTryWithResources() {
    OnClose onClose = new OnClose();
    try(Stream<String> stream = getStream(onClose)) {
        stream.forEach(System.out::print);
    }
    // closedを呼び出していれば trueになる。
    assertThat(onClose.isCalled(), is(true));
}

さて、このテストを実行すると、二つのテストはパスします。

これが意味していることは、終端処理を実行するだけではcloseメソッドが呼ばれないということです。

そのため、サンプルとして提示されている下記のコードにおいては

Files.lines(Path).forEach(System.out::println);

br.lines().onClose(asUncheckedRunnable(br))で指定されたasUncheckedRunnableで返されるRunnableの処理は実行されません。

つまり、ファイルリソースの解放もれが発生します。

ここで、さらに

ファイル読み書き系のバッチ処理において極めて有効

と記述されており、もしかしたら大量のファイルのファイルリソースの解放漏れがあるのではないかと考えられます。

StreamがAutoClosableを実装していることについては、Javaプログラマーなら習得しておきたい Java SE 8 実践プログラミングでも指摘されていることです。また、その書籍でもFiles#lines(Path)はtry-with-reosurcesで受け取ることとされているので、便利だからといってリソースの解放もれという落とし穴があることには気をつけないといけません。

---

以上、家で精神の安定を欠いて、JJUG CCCに行かなかった僕が後出しジャンケンで言うのもはばかられる気がしなくもないですが、後々の人がはまらないために(楽天はどうだか知らん)、これだけはちゃんと言っておきたかった。

---

宣伝

今、僕はうつ病(季節性を含む/障害者2級)、大人の発達障害(優先順位が判断できないなどビジネス上基本的なスキルが欠ける)で、フリーでも仕事を断られ続けてて、経済的に困窮しています。雇ってくれる企業さん(障害者枠)、あるいは週15時間程度でお仕事をくれる人(時給換算3,000円以上)を募集しています。もし雇ってもいいよ、仕事あげるよって方がおられましたら、ご連絡をいただけると大変ありがたいです。

---

【追記 2015/04/12 22:24】

ツイートたどってたら、こんなんあった。

多分、発表者の方だと思う。

---

そこんところはtry-with-resourceで囲むことは言いたいことの本質じゃないからわざと省いてるんだけど… 39Pくらいでちゃんと書いてるがなｗ

---

普通に考えたら省いたことくらい分かりそうだけどな。

「もしかしたら大量のファイルのファイルリソースの解放漏れがあるのではないかと考えられます」って文にはさすがにちょっと御社の技術力舐めすぎじゃろと思った

---

さすがにFile開いてCloseしないは素人すぎるｗ まぁいいってことよ もっとネガティブなアレがあるかと思ってたし

---

僕は会場にいなかったので、その説明があったのかどうかわからんのだけど、本質的でないところは端折ったよ、ちゃんと行間読んでねで終わらせるのではなく、ちゃんと「目立たないけど39Pに書いてあります( ｰ`дｰ´)ｷﾘｯ」と指摘して欲しかった。マサカリに対する耐性は僕にはないけど、内輪だけで「御社の技術なめすぎ、こいつｗｗｗ」とか言ってないでちゃんと僕のミスリーディングに対してマサカリ投げて、コミュニティ全体に情報を発信してほしいものです。発表者「@mike_neck おい、39Pに書いとるがな。弊社の技術なめんなや！」→儂「おお、これは気づきませんでした。では、このブログでちゃんと説明しておきますね。」っていう流れで、公開している情報がより強化される機会が生まれるはずだったわけですから。

【2015/04/13 8:11】 ツイッター垢特定するのは良くないので、垢の情報取り除いた