LoginSignup
9
11

Javaアンチパターン:トラブルの原因になるマズいコード

Last updated at Posted at 2022-02-22

この記事は,かつてdeveloperWorksの「WebSphere Application Server小ワザ集」でApplication Anti Patternとして公開されていたものです。筆者が過去に経験したコードに原因があったトラブルを,どのようなコードが悪くてトラブルになったのかをまとめた記事です。トラブルはWebSphere Application Server上で発生したものですが,他の多くのJava EE環境でも同様の問題が発生すると思います。

内容は相当に古いのですが,現在の環境でも役に立つ内容もあるので,再掲載しようと思います。「現在では」の部分以外は,タイプミスやコードのバグ以外は,原則として当時のまま記載しています。

データベース・コネクションの確保,利用,クローズがコード中に分散している

アンチパターン

あるお客様のコードでは,データベースを使用する際のConnectionに対する処理がコード中のあちこちに分散していました。つまり,Connectionを取得する部分,Connectionを利用してデータベースの参照・更新をおこなう部分,Connectionをクローズする部分が,それぞれ別のクラス/別のメソッドで定義されていて,それらの間には複数の処理が記述されていました。

結果

このコードでは,データベース利用の間の処理でExceptionが発生した場合,処理が中断して取得したConnectionのクローズが行われないという現象が発生してしまいました。

そのため,しばしばコネクションプール中のデータベース・コネクションが使い尽くされてしまい,アプリケーション・サーバーが使用不能になるという事態が頻発しました。

ベストプラクティス

データベース・コネクションの確保,利用,クローズは必ず一カ所でおこなうようにしてください。その際には取得・利用のコードを必ずtry節で囲み,クローズ処理はそのfinally節で行うようにしてください。これにより,確保されたデータベースコネクションが確実にクローズされるようになります。

Connection conn = null;
try {
    conn = dataSource.getConnection(USERID, PASSWORD);

    // データベースに関する処理

    conn.close();
} catch (SomeException ex) {
    // Exceptionに対する処理
} finally {
    try {
        if (conn != null) conn.close();
    } catch (Exception ex) {}
}

現在では

最近のWebSphere Application Serverなどのアプリケーションサーバーでは,DBのコネクションを監視していて,サーブレッド等の処理が終了してもクローズされていないConnectionを検出すると,自動的にクローズ処理をおこなうようになっています。そのため,これが原因でコネクションプールが枯渇するようなことはなくなっています。それでも,エラー処理などを正しくおこなうようにするためには,Connectionのクローズを確実におこなうようにすることは重要です。

また,Java 7以降で導入されたtry-with-resources文をつかうと,上記のコードは以下のように簡潔にかけるようになっています。クローズ処理は,try節をぬけるさいに自動的におこなわれます。

try (Connection conn = dataSource.getConnection(USERID, PASSWORD)) {

    // データベースに関する処理

} catch (SomeException ex) {
    // Exceptionに対する処理
}

Exceptionのコンストラクタに処理を記述する

アンチパターン

あるユーザーは,ユーザーExceptionクラスを実装した際に,そのコンストラクタ自身にロギングを行わせる処理を実装しました。ところが,ここで使ったロギングAPI自体もユーザーアプリケーションの一部だったので,エラー発生時には同じユーザーExceptionを発生する可能性がありました。

結果

このロギングAPIがエラーを発生する状況で,深刻な問題が発生しました。

ユーザーExceptionが発生するたびに,「Exceptoinのコンストラクタの実行」 -> 「ロギングAPIの実行」 -> 「Exceptionの発生」 -> 「Exceptoinのコンストラクタの実行」・・・,の無限呼び出しが発生してしまい,JVMがダウンしてしまいました。

ベストプラクティス

ユーザー・アプリケーション独自のExceptionを作成した場合,そのコンストラクタにはできるだけロジックを書かないようにしてください。また,コンストラクタやメソッドにロジックを組み込む場合,その内部ではJavaの標準APIや外部ライブラリのAPIのみを用いて処理を行い,ユーザーアプリケーションの他のクラスの機能を用いた処理は決して書かないでください。

このような書き方をすると,ユーザーException内部で同じExceptionが発生する危険があります。この場合,無限の再帰呼び出しが発生してしまい,スタックを使い尽くしてJVM自身のダウンを引き起こすことがあります。

Exceptionに対する処理(ロギングなど)は,そのException自身にさせるのではなく,かならずcatchしたメソッドによって行うようにします。

現在では

昔のJVMは,スタックを使い尽くすとすぐプロセスダウンを引き起こしていたのですが,最近のJVMはきちんとStackOverflowErrorをスローするようになってくれています。ただ,このエラーを引き起こす悪い書き方である事は変わりませんので,やめましょう。

外部リソース利用の繰り返し

アンチパターン

あるお客様が,アプリケーションの挙動を詳細に調査するため,自作のロギングクラスを作成し,アプリケーション内の各所に追加されました。

このロギングクラスは,ログを記録するメソッドが呼ばれるたびに,ファイルをオープンし,ログを書き込み,クローズする,というつくりになっていました。

結果

クライアントからのリクエストが一回行われるたびに,このロギング・メソッドは数回~十数回呼び出されるようになっていました。

ファイルのオープン・クローズという処理はかなり重い処理です。

このため,アプリケーション全体のパフォーマンスがかなり低下してしまいました。特に,ログを記録するメソッドがsynchronized指定されていたため,多くのクライアントからリクエストが集中したときには処理が直列化され,著しいパフォーマンスの劣化を招いてしまいました。

ベストプラクティス

外部のリソース,たとえばファイル,ネットワーク接続,DB接続などを使用する際には,使用前にオープン,使用後にクローズの処理が必要になります。このオープン・クローズの処理は,多くの場合において非常に大きなコストのかかる重い処理です。これらの処理は極力減らすようにコーディングする必要があります。

例えば今回の例では,ファイルはロギングを担当するマネージャークラスがオープンして保持しておき,個々のログの記録が行われるときには直接ログをファイルに書き込むのではなく,マネージャークラスへログの通知を行うような設計にするべきでした。ファイルへの書き出しは,マネージャークラスが一括して,一定時間ごと,あるいは一定回数ごとに非同期的に行うようにしておけば,パフォーマンスの劣化は最小限にすることができます。

また,共有可能な外部リソースについては,プーリングによる再利用をおこなって,リソースのオープン/クローズの回数を減らすことも効果があります。

現在では

この例ではロギング機能を自作されていましたが,現在では多くの高機能なロギング機能がJava標準やOSSなどで提供されていますので,直接同じ問題はおこらないでしょう。ですが,繰り返し使用される部分にリソースのオープン・クローズ処理があると,パフォーマンスが劣化することは変わりません。

操作する際にロックがかかるクラスをHttpSessionにいれる

アンチパターン

あるアプリケーションは,HttpSessionjava.util.Collections#synchronizedSet()メソッドで作成したSynchronizedSetを格納していました。

また,別の箇所ではHttpSessionから取り出したSynchronizedSetにロックをかけたsynchronizedブロックがあり,そのなかでHttpSessionの操作をしていました。

結果

このアプリケーションは,シングル環境で動作させているうちは問題ありませんでした。ですが,複数アプリケーションサーバーからなるクラスター環境で動かしたところ,内部でデッドロックが発生してアプリケーションが停止してしまいました。

デッドロックの発生メカニズムはこうです。

クラスター環境では,アプリケーションサーバー間でセッションを共有するため,HttpSessionの内容物をシリアライズして外部に送信します。この処理を行う際に,WAS(WebSphere Application Server)のセッションマネージャーは対象のHttpSessionをロックします。またSynchronizedSetはシリアライズされる際に自身のロックをかけるようになっています。そのため,セッションの送信時にHttpSession→SynchronizedSetの順で二重ロックがかかります。

一方アプリケーションではSynchronizedSetにロックをかけた状態でHttpSessionを操作しています。その操作のなかには,WAS内部でHttpSessionにロックがかかる処理がありました。そのためSynchronizedSet→HttpSessionの順で二重ロックがかかっていました。

Javaプログラムの中に,異なる順序で二重ロックをかける箇所があると,タイミングによってデッドロックが発生してしまいます。特にこのケースではデッドロックの片方がWASのセッションマネージャーであったため,他のスレッドからもHttpSessionの操作ができなくなるなど深刻な障害となってしまいました。この状況は,アプリケーションサーバーを再起動するまで解消されませんでした。

ベストプラクティス

HttpSessionに,操作の際にロックがかかるオブジェクトを格納する際には注意が必要です。とくにtoString()やhashCode(),writeObject()メソッドなどが同期化(synchronized)されている場合に問題が発生する可能性が高くなります。

このようなオブジェクトとしては,Collections#synchronizedSet()やCollections#synchronizedMap(),Collections#synchronizedCollection()などで作成したオブジェクトや,StringBufferのインスタンスなどがあります。

このようなオブジェクトを格納している際には,それらのオブジェクトにロックをかけたままHttpSessionの操作をしないで下さい。

またHttpSessionを安全につかうためには,なるべくStringやIntegerなどの変更不能クラス(Immutable Class)のインスタンスだけを格納するようにします。アプリケーションでロックの対象としているオブジェクトへの参照を,フレームワークやライブラリなど外部のプログラムで保管することも極力避けるようにしましょう。

現在では

昨今ではオブジェクトを可能な限り不変クラスとして実装することが,コーディングのベストプラクティスとして共通認識されるようになっているかと思います。それは,このようなトラブルを避けるためにも役立ちます。

親クラスのstaticメソッドを利用するためだけにそのクラスを継承

アンチパターン

あるクラスには,Thread#currentThread()を使用する処理が何回も記述されていました。そのクラスを作成したプログラマーは,タイプ量を減らすため,そのクラスの定義に「extends Thread」を追加しました。これにより「Thread.currentThread()」と書くところが「currentThread()」と書くだけですむようになりました。

結果

そのクラスのインスタンスが利用されるたびに,開放されないヒープエリアがだんだんと増えていくという現象が発生しました。メモリリークが発生したのです。

ベストプラクティス

java.lang.Threadとそれを継承したクラスのインスタンスは,その実行が完了するまでの間,たとえ他からの参照が一切なくなってもGCの対象となりません。

このクラスでは,start()によるスレッド実行の処理は全く記述されていませんでしたので,そのインスタンスは実行完了前のスレッドのオブジェクトとみなされ,GCが行われても全く開放されない状態になっていました。

Threadクラスに限らず,一般に親クラスを継承すると,クラスのインスタンス作成に伴って,親クラスで定義されているフィールドなど各種リソースも作成されてしまいます。それらを一切使わないのでは,継承をする意味がありません。

オブジェクト指向の一般手法に従い,「is-a」関係にある場合にのみ継承を用いて,その他の目的には使わないでください。

現在では

別のクラスのstaticメンバやstaticメソッドを,クラスの名前で修飾しなくても利用できる手段として,staticインポートという仕組みがJava 5から追加されました。

そのため,ソースのimport定義の中に以下の記述を追加すれば,クラスを継承することなく
「currentThread()」でメソッドを呼び出せるようになります。

import static java.lang.Thread.currentThread;

外部リソースにアクセスするためのパラメーターをハード・コーディングしている

アンチパターン

あるプログラムには,データベースへの接続を提供するDataSourceをJNDIからLookupするためのパラメーターが,クラスのstatic変数としてハード・コーディングされていました。また,パラメーターは使用するクラスごとに定義していたため,値が埋め込まれたクラスは多岐にわたっていました。

結果

このプログラムでは,アプリケーション・サーバーのバージョンをあげる際に,コード全体にわたって大規模な修正が必要になってしまいました。また,サーバーの環境を変更した際にも,各種の接続障害によるエラーが発生し,なかなか解決できませんでした。

ベストプラクティス

データベースに対する接続パラメーターに限らず,外部のリソースにアクセスするためのパラメーターは,使用しているJ2EE規格やWASのバージョンが上がったりすると,しばしば変更の必要性が発生します。また,アプリケーションを運用しているサーバーの構成を変更した場合などにも,接続のためのパラメーターは変化する可能性があります。

たとえば,WASの提供するJNDIのInitialContextを取得するためのファクトリー・クラスは,WAS V3.5以前では,

com.ibm.ejs.ns.jndi.CNInitialContextFactory

でしたが,WAS V4.0以降では,

com.ibm.websphere.naming.WsnInitialContextFactory

に変更になっています。

また,アプリケーションからJNDIによりオブジェクトをlookupする際には,WAS V3.5以前ではGlobal JNDI名を直接参照していましたが,WAS V4.0以降ではJ2EEでの規定に従い,リソース参照を使用してLocal JNDI名で参照することが推奨されています。

このような外部のリソースに接続するためのパラメーターとしては,以下のようなものがあります。

  • JNDIで使用するInitialContextを取得するためのファクトリー・クラスやプロバイダーURL
  • JNDIでlookupするオブジェクトのJNDI名
  • 使用するローカルファイルのディレクトリー名/ファイル名
  • HttpConnectionで接続する外部HTTPサーバーのURL
  • Socketで接続するサーバーのホスト名とポート名

このようなパラメーターについては,クラスのstatic変数などでハード・コーディングするのではなく,デプロイメント・ディスクリプターの環境エントリーや,プロパティー・ファイルを利用して一括管理し,アプリケーションの再コンパイルなしに修正が可能なようにしておくべきです。

また,Jakarta Common LoggingなどのロギングAPIを使用している場合には,実際に値を取得して使用するときに,トレースのレベルでパラメーターを記録するようにしておくと問題が発生したときに解決が容易となります。

現在では

オブジェクトの取得をコードとして書くのではなく,構成によって外部からインジェクションするDIの技術が,Java EE/Jakarta EEにも広く取り入れられています。これらを活用することにより,コードの環境依存性を大きく減らすことができるようになっています。またMicroProfileのConfig APIなど,構成要素を外出しにして複数サーバー間で共有する仕組みなども提供されるようになっています。

リクエスト処理中にグローバルなHashtableにアクセスする

アンチパターン

あるプログラムは,プログラム全体で使用する構成情報を単一のHashtableで管理していました。プログラムは,リクエストを処理する過程で,このHashtableに格納されてる情報を随時読み出して利用していました。

結果

このプログラムは,シングルコアや少数のコアをもつのサーバー上で実行している間は特に問題は見られませんでした。しかし,多くのコアを持つサーバー上で実行し,高い負荷をかけた際に,深刻なパフォーマンス低下を引き起こしてしまいました。

ベストプラクティス

多くのコアを持つサーバー上で実行するアプリケーションにおいては,並行実行性(Concurrency),つまり複数の処理が同時に実行可能になっていることが非常に重要となります。並行実行性が悪いアプリケーションであっても,単一のコアの環境で実行している間はほとんど問題なく動きます。ですが,このようなアプリケーションはコア数が多くなってもパフォーマンスが向上せず,場合によってはコアが増えるにしたがってパフォーマンスが悪化することすらあります。

排他制御のためにおこなわれるオブジェクトのロックは,並行実行性を損ねる代表的な原因です。Javaではオブジェクトをロックするためにsynchronized構文が使用されます。アプリケーション中でsynchronizedされた箇所を必要最小限にとどめることが,並行実行性を保つために必要です。

アプリケーションの中で明示的にsynchronized構文を使用していなくても,オブジェクトにロックがかかる場合があります。代表的な例が標準APIで提供されているHashtableです。このクラスのメソッドは全て同期化されていて,値を追加したり読み出したりすることにロックがかかります。アプリケーションで処理ごとにHashtableの読み出しをおこなっていると,並行実行性の極めて悪いアプリケーションになってしまいます。

アプリケーションの処理ごと利用される値を格納する場合には,読み書きに際してロックのかからないHashMapを利用するようにします。また,JDK 5以降を使用している場合には,ConcurrentHashMapも使用できます。ConcurrentHashMapは,書き込みではロックをかけて整合性を保ちつつ,読み出しではロックを使用せずに値にアクセスできます。

また,同じような例としてSystem.getProperty()によるシステム・プロパティの読み出しがあります。システム・プロパティは,内部でHashtableを使用して保存されているため,読み出すたびにロックが発生してしまいます。このような場合には,処理の初期化時にあらかじめシステム・プロパティを読み込んで変数に保存しておき,処理ごとの参照はその変数経由でおこなうようにします。

現在では

この問題が顕在化する「多くのコア」とは,32コアとか48コアとかの環境です。昨今ではVMやコンテナなどの仮想化技術が発達したため,多くのコアを持つサーバーを単一のOSで利用することは少なくなり,この問題が顕在化することも少なくなってきました。

が,現在のJava 17でもHashtableやPropertiesが読み出しを含めて同期化される実装である事は変わりません。同期処理のコストについては,頭の隅においておきましょう。

(2022年10月追記)Java 11からは,java.util.Propertiesは,値の保存に継承元のHashtableを使うのではなく,自身で保持するConcurrentHashMapをつかうように内部実装が変更されていました。Java 11以降では,System.getProperty()による複数スレッドの排他制御は発生しないようになっています。

HttpSessionに入れたオブジェクトを外部から変更する

アンチパターン

アプリケーションはHttpSessionに変更可能なオブジェクトを格納していました。あるサーブレットの中で,すでに格納されているこのオブジェクトの内容を変更する操作が実装されていました。この操作は,「オブジェクトをHttpSessionからgetAttributeメソッドで取り出す」「オブジェクトの内容を変更する」という処理が実装されていましたが,「変更したオブジェクトをsetAttributeメソッドで再格納する」という処理は実装されていませんでした。

AccessTime accessTime = (AccessTime)session.getAttribute(ATTRIB_NAME_ACCESSTIME);
accessTime.setLastAccessTime(System.currentTimeMillis());

結果

このアプリケーションは,単一のアプリケーションサーバーで実行されている時には問題を起こしませんでした。しかしHttpSessionを複数サーバーで共有しているクラスター環境で使用した場合に問題が発生しました。サーバー障害でFailoverしたとき,あるいは運用の都合でサーバーが停止されたとき,引き継ぎ先のサーバーでHttpSessionから取り出したオブジェクトには,引き継ぎ前に実行された変更内容が反映されていませんでした。

ベストプラクティス

複数のWASアプリケーションサーバーでHttpSessionを共有している場合には,HttpSession外でオブジェクトを変更したとしても,その変更内容は他のアプリケーションサーバーに反映されません。変更を行った場合には,かならずsetAttributeメソッドでオブジェクトを更新する必要があります。

上記のコードは,正しくは以下のようなコードに変更する必要があります。

AccessTime accessTime = (AccessTime)session.getAttribute(ATTRIB_NAME_ACCESSTIME);
accessTime.setLastAccessTime(System.currentTimeMillis());
session.setAttribute(ATTRIB_NAME_ACCESSTIME, accessTime);

JSPファイル内でjsp:useBeanタグでsessionスコープからオブジェクトを取得している場合も,オブジェクトの内容を変更する処理を行った場合には,かならずsetAttributeを呼び出してオブジェクトの更新をHttpSessionに通知してください。

<jsp:useBean id="pageHistory" class="com.ibm.jp.PageHistoryBean" scope="session" />
<%  pageHistory.setLastAccessPageID("JSID0023");
    session.setAttribute("pageHistory", pageHistory); %>

また,このようなトラブルを避けるためにも,HttpSessionには可能な限り変更不能なオブジェクト(Immutableなオブジェクト)のみを格納することをおすすめします。JavaのStringクラスやIntegerやDoubleなどのラッパークラスのインスタンスは,代表的な変更不能オブジェクトです。

9
11
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
9
11