make Session.shutdown()/close, Transaction.close(), and PreparedState… #501
Conversation
…ment close() execute asynchronously
| @@ -39,32 +39,55 @@ public class Disposer extends Thread { | |||
|
|
|||
| private AtomicBoolean started = new AtomicBoolean(); | |||
|
|
|||
| private AtomicBoolean sessionClosed = new AtomicBoolean(); | |||
| private Queue<ForegroundFutureResponse<?>> futureResponseQueue = new ArrayDeque<>(); | |||
There was a problem hiding this comment.
キューを使う箇所でsynchronizedしていますが、ConcurrentLinkedQueueを使えばいいのではないでしょうか。
There was a problem hiding this comment.
ConcurrentLinkedQueueを使いました。
| private AtomicBoolean sessionClosed = new AtomicBoolean(); | ||
| private Queue<ForegroundFutureResponse<?>> futureResponseQueue = new ArrayDeque<>(); | ||
|
|
||
| private Queue<DelayedClose> serverResourceQueue = new ArrayDeque<>(); |
There was a problem hiding this comment.
ここも同様（ConcurrentLinkedQueue）
| } catch (TimeoutException e) { | ||
| // Let's try again | ||
| queue.add(futureResponse); | ||
| futureResponseQueue.add(futureResponse); |
There was a problem hiding this comment.
futureResponseQueueを（他の個所ではsynchronizedしているのに）synchronizedせずに使ってますね
There was a problem hiding this comment.
ConcurrentLinkedQueueを使いました。
| synchronized (queue) { | ||
| sessionClosed.set(true); | ||
| return queue.isEmpty(); | ||
| public synchronized void registerDelayedShutdown(DelayedShutdown c) throws IOException { |
There was a problem hiding this comment.
このメソッドの中のc.shutdown()に長時間かかる可能性があるので、
このメソッドでsynchronizedすると、他スレッドから同時に呼び出された場合に待ちが発生しますが、大丈夫でしょうか？
’（呼び出し元は FutureResponse<Void> shutdown(@Nonnull ShutdownType type) であり、Futureを返すメソッドはなるべく早く戻るべきな気がします（future.get()で時間がかかるのは普通のことなので気にならない））
（それとも、shutodwnメソッドは時間がかかっても構わないという仕様？）
There was a problem hiding this comment.
c.shutdown()はsynchronized(this)の外に出しました。
| } | ||
| shutdown.set(c); |
There was a problem hiding this comment.
複数スレッドから同時に呼ばれた場合や複数回呼ばれた場合、shutdownにセットされるのはひとつだけになりますが、問題ないでしょうか？
（呼び出し元はSessionImpl#shutdown()なので、ユーザーが異なる引数で複数呼ぶことは可能）
There was a problem hiding this comment.
複数のshutdownをセット可能にしました。
| } | ||
|
|
||
| @Override | ||
| public void setCloseTimeout(long t, TimeUnit u) { | ||
| timeout = t; | ||
| unit = u; | ||
| synchronized (this) { |
There was a problem hiding this comment.
synchronized(this) しか無いメソッドなら、メソッドをsynchronizedにした方がいいと思いますが、
たぶんここで排他したいのはthisではなくタイムアウトフィールドですよね？
thisで排他するとcommitやcloseと競合しますが、setCloseTimeoutを複数スレッドから呼ぶことは考えにくいので、実際的には問題にならないような気もしますが。
There was a problem hiding this comment.
メソッドをsynchronizedにしました。synchronizedは不要と思ったのですが、spotBugsでエラーになるので入れています。
| return sendAndGetSqlServiceException(); | ||
| } | ||
| } | ||
| return sendAndGetSqlServiceException(); | ||
| } |
There was a problem hiding this comment.
synchronized(this)の終わりがここですが、この排他は実質的にstateの排他だと考えられるので、
var cr = commitResult の前まで排他すれば良いように思われます。
そして、state自身がAtomicなので、CLOSEDかどうかをチェックするだけなら、synchronized自体不要かも？
（もしstateの排他という意味なら、synchronized(this)よりsynchronized(state)の方が分かりやすいですが（他のメソッドも含めて））
There was a problem hiding this comment.
commitResultも排他の対象になっています。また、synchronized(state)としたかったのですが、SpotBugsがエラーだと誤検出してしまいます。
| @@ -279,11 +326,79 @@ public String getTransactionId() { | |||
| return transaction.getTransactionId().getId(); | |||
| } | |||
|
|
|||
| private State toBeClosed(State s) { | |||
There was a problem hiding this comment.
呼ばれるメソッドは、呼ぶメソッドの下にある方が分かりやすいです。
（Javaにおける、ソースコードの慣例）
| } | ||
|
|
||
| private void doClose() throws IOException, ServerException, InterruptedException { | ||
| synchronized (this) { |
There was a problem hiding this comment.
doClose()はclose()からしか呼ばれないので（closeの中でsynchronized(this)されているので）、ここでの排他は不要です。
（不要である旨はソースコード上にコメントとして残した方がいいかもしれませんが）
There was a problem hiding this comment.
doClose()はDisposerのスレッドからcallされる可能性があります。
| // need session.close() followed by SessionImpl.waitForDisposerEmpty() | ||
| session.close(); | ||
| if (session instanceof SessionImpl) { | ||
| ((SessionImpl) session).waitForDisposerEmpty(); |
There was a problem hiding this comment.
iceaxe-dbtestでもDisposerが終了したことを確認した方がいいと思われるので、Sessionから呼べると嬉しいです。
（IceaxeでSessionImplを使うのはいまいちなので）
There was a problem hiding this comment.
これは議論が必要そう。waitForDisposerEmpty()はtestだけで使うAPI（Arch meetingの決定事項）なので、Sessionが持つAPIとするのは不適切という気がしました。testのみのAPIもSessionが持つAPIに持たせてよいのであれば、そのように変更します。
https://github.com/project-tsurugi/tsurugi-issues/issues/1022