正しく使うことを期待する vs 意識せずとも正しく使えるようにする

仕事でリソースリーク起こしてシステムが止まったという事案を受けていろいろ考えてた。
この問題が起きた時、実装としては「正しく使ってもらうことを期待する」実装になっていた事に気づいたのでこの点で1本書きたくなった。

ちなみに、今回問題を起こしたのはコネクションプールにちゃんと戻さないことがあったのが原因だった。これを問題が起きない形にするにはどうするか考えました。

1. 問題のコード(抜粋)

こんな感じになっていて、executeを実装したクラスを扱うコードになっている。
SingletonConnectionPoolは名前の通りコネクションプールで、クラスインスタンスの内部にpoolを保持していてそこからやりくりする。当然スレッドセーフです。

@Override
public void execute() throws Throwable{
    Connection con = null;
    try{
        con = SingletonConnectionPool.getConnection();
         // connectionを使った処理
        con.commit();
    }catch(Throwable e){
        if(con != null){
           con.rollback();
        }
        throw e;
    }finally{
        if(con != null){
            SingletonConnectionPool.back(con);
        }
    }
}

しかし、finally句の部分を付け忘れていたクラスがあった。
poolからとった情報はちゃんと戻すのがルールで、finally句の部分がないというのは、それを守れなかったことになる。

使い方を守って正しく使いましょうといえばそれまでなんだけど、それで正しく扱えずに今回の問題が起きたのだから、それ以外で考えたい。
コードレビュー()も通ったコードなのに、こういったミスは起こる。それはレビュアーやレビュイーがそこまでの注意を払えてなかったりするから。どのクラスでも同じことやってるよねーって程度の認識しかなかったとか。(数百も同じようなクラスがあれば1個は見落とすでしょう？僕は見落としました)
単純で同じ確認作業を何回もしなくてはならない場面においては人はあまりあてにならない。

2. テンプレートメソッドパターンを使う

処理部分だけ違うのだから、ここだけ切り出してこういう形にすれば、全体で同じ手順にできるので良いのでは。
デザインパターンを知っていれば思いつくと思う。(そもそも元々のメソッド(execute)がテンプレートメソッドなんだけども・・・)

@Override
public void execute() throws Throwable{
    Connection con = null;
    try{
        con = SingletonConnectionPool.getConnection();
        mainLogic(con);
        con.commit();
    }catch(Throwable e){
        if(con != null){
           con.rollback();
        }
        throw e;
    }finally{
        if(con != null){
            SingletonConnectionPool.back(con);
        }
    }
}

abstract public void mainLogic(Connection con); // connectionを使った処理

利用者はmainLogicに実装をするだけ。Connectionは使える状態のものが引数で渡ってくるし、問題があった時や後始末は勝手にやってくれる。
ただ、これには自由がない。実はConnectionは全く使いません、といった要件にはマッチしない。Connectionを扱うのとそうでないのとで分ければよい、という話もあるが。

3. テンプレートメソッドではなく、そういったクラスを提供する

こんなクラスを準備しておく。

public interface ConnectionRunnable{
    void run(Connection con) throws Throwable;
}

public class ConnectionExecutor{
    public static void execute(ConnectionPool connectionPool, ConnectionRunnable runner) throws Throwable{
        Connection con = null;
        try{
            con = SingletonConnectionPool.getConnection();
            runner.run(con);
            con.commit();
        }catch(Throwable e){
            if(con != null){
               con.rollback();
            }
            throw e;
        }finally{
            if(con != null){
                SingletonConnectionPool.back(con);
            }
        }
    } 
}

元のコードはこうする。

@Override
public void execute() throws Throwable{
    ConnectionExecutor.execute(SingletonConnectionPool.getInstance(), (con) -> {
         // connectionを使った処理
    });
}

Lambdaを使ってわざとらしく書いているが、大分シンプルになるし、間違えてconnectionをpoolに戻し忘れることもない。
Connectionを使わないのであれば、new ConnectionExecutor()といった文言が出てこなくなるので、意図もわかりやすい。
難点はここだけしか使えないクラスが増える。

こういう書き方をあまり見ないので、自分はテンプレートメソッドより良いと思っているのだけど、周りから同意が得られるかどうかはわからない。

まとめ

1のやり方は、何でもできるやり方なので実装者が正しく実装することを期待した方法といえる。
2,3は制限がつくものの、意識せずとも正しく実装できるようになっている。

元々、1で起きうる問題を防げるような実装にできないかと考えて2つの案を考えたが、どちらも少なからず自由度を犠牲にすることになる。2,3のやり方は言わばレールの上を走るようなもので、レールから外れたことをしようとすると途端に難しくなる。最悪、2,3のやり方では実装できない可能性もある。たとえば2個のコネクションを1つの処理で扱わなければならなくなったとき2,3では難しい。1のやり方であれば単純に増やすだけでできそうだ。

全員が常に正しく実装できるなら、1の形で実装してしまってもよい。
不特定多数の人に実装してもらう必要が出るのであれば、とりあえずでもよいので2,3の形で実装してしまったほうが良いと思う。これで対応できないケースが出たとき、問題が起きた時はその時に考える。少しコストで問題を防げるなら安いと思うので。

理想と現実

現実的な問題として、実際の現場で今更書き直すことは許されないだろう。
今のコードで動いてるのに書き換えるメリットは？と言われると上記のことから「前回の問題を起きないようにできる」ぐらいしか言いようがない。これらは実装前に考えておくべき問題で、問題が起きた後の対処では遅いのです。

結果として同じなのだから、動いているコードを変える必要がない、と判断するのは正しい。動いているコードというのは少なからず金を生み出しているので、それを不用意な変更で危険に晒したくないのが上が考えることだ。修正にはコストもかかる。そしてそのコストは今払うべきでないと判断する。
ここは管理職がどう判断するかによるが、多分自分でも同じ状況なら、今問題になってる箇所を他に合わせて修正するだけに留めるよう判断すると思う。昔の自分なら1はクソコードなので殲滅しろ一蹴していたと思うが、人は環境に居続けると適応してしまうのである。悲しい。

終わり

やり方はいろんな方法もあるし、ちょっと工夫すれば制限付きで問題を防げる仕組みを作れる。

最後に、人を信用して成り立つ設計は正しくないというのは付け加えておきたい。どんだけ人間不信なんだってくらいが設計者としてはちょうど良いとも思える。