From 25e91f99a36e504adb4e7a74d77e264b6a5898b3 Mon Sep 17 00:00:00 2001 From: Gerd Behrmann Date: Thu, 5 Dec 2013 14:40:45 +0100 Subject: [PATCH] ftp: Fix deadlock introduced in recent refactoring Recent changes to the FTP door introduced a deadlock: "pool-116-thread-7": at org.dcache.ftp.door.AbstractFtpDoorV1.setTransfer(AbstractFtpDoorV1.java:2380) - waiting to lock <0x00000007c5eade98> (a org.dcache.ftp.door.GsiFtpDoorV1) at org.dcache.ftp.door.AbstractFtpDoorV1$FtpTransfer.transferCompleted(AbstractFtpDoorV1.java:1022) - locked <0x00000007c5eb3860> (a org.dcache.ftp.door.AbstractFtpDoorV1$FtpTransfer) at org.dcache.ftp.door.AbstractFtpDoorV1$FtpTransfer.finished(AbstractFtpDoorV1.java:972) at org.dcache.util.Transfer.finished(Transfer.java:464) - locked <0x00000007c5eb3860> (a org.dcache.ftp.door.AbstractFtpDoorV1$FtpTransfer) at org.dcache.ftp.door.AbstractFtpDoorV1.messageArrived(AbstractFtpDoorV1.java:1444) at sun.reflect.GeneratedMethodAccessor132.invoke(Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:606) at org.dcache.cells.CellMessageDispatcher$ShortReceiver.deliver(CellMessageDispatcher.java:289) at org.dcache.cells.CellMessageDispatcher.call(CellMessageDispatcher.java:202) at org.dcache.cells.AbstractCell.messageArrived(AbstractCell.java:859) at dmg.cells.nucleus.CellAdapter.messageArrived(CellAdapter.java:981) at dmg.cells.nucleus.CellNucleus$DeliverMessageTask.innerRun(CellNucleus.java:1037) at dmg.cells.nucleus.CellNucleus$AbstractNucleusTask.run(CellNucleus.java:946) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615) at java.lang.Thread.run(Thread.java:744) "Thread-160": at org.dcache.ftp.door.AbstractFtpDoorV1$FtpTransfer.abort(AbstractFtpDoorV1.java:1060) - waiting to lock <0x00000007c5eb3860> (a org.dcache.ftp.door.AbstractFtpDoorV1$FtpTransfer) at org.dcache.ftp.door.AbstractFtpDoorV1$FtpTransfer.abort(AbstractFtpDoorV1.java:1036) at org.dcache.ftp.door.AbstractFtpDoorV1.shutdown(AbstractFtpDoorV1.java:1363) - locked <0x00000007c5eade98> (a org.dcache.ftp.door.GsiFtpDoorV1) at diskCacheV111.doors.LineBasedDoor$CommandQueue.stop(LineBasedDoor.java:343) - locked <0x00000007c5ed1290> (a diskCacheV111.doors.LineBasedDoor$CommandQueue) at diskCacheV111.doors.LineBasedDoor.run(LineBasedDoor.java:176) at java.lang.Thread.run(Thread.java:744) This deadlock is caused by unnecessary synchronization in the shutdown method. This synchronization did not exist in the old code. The patch also adds an accessor for the transfer field, thus solving potential race conditions. Target: trunk Require-notes: no Require-book: no Acked-by: Tigran Mkrtchyan Patch: http://rb.dcache.org/r/6318/ --- .../dcache/ftp/door/AbstractFtpDoorV1.java | 32 ++++++++++--------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/modules/dcache-ftp/src/main/java/org/dcache/ftp/door/AbstractFtpDoorV1.java b/modules/dcache-ftp/src/main/java/org/dcache/ftp/door/AbstractFtpDoorV1.java index 433b3a54315..81c3981ddce 100644 --- a/modules/dcache-ftp/src/main/java/org/dcache/ftp/door/AbstractFtpDoorV1.java +++ b/modules/dcache-ftp/src/main/java/org/dcache/ftp/door/AbstractFtpDoorV1.java @@ -1315,7 +1315,7 @@ public Object ac_get_door_info(Args args) long[] uids = (_subject != null) ? Subjects.getUids(_subject) : new long[0]; doorInfo.setOwner((uids.length == 0) ? "0" : Long.toString(uids[0])); doorInfo.setProcess("0"); - FtpTransfer transfer = _transfer; + FtpTransfer transfer = getTransfer(); if (transfer != null) { IoDoorEntry[] entries = { transfer.getIoDoorEntry() }; doorInfo.setIoDoorEntries(entries); @@ -1365,14 +1365,12 @@ public void ftpcommand(String cmdline) // If a transfer is in progress, only permit ABORT and a few // other commands to be processed - synchronized(this) { - if (_transfer != null && + if (getTransfer() != null && !(cmd.equals("abor") || cmd.equals("mic") - || cmd.equals("conf") || cmd.equals("enc") - || cmd.equals("quit") || cmd.equals("bye"))) { - reply("503 Transfer in progress", false); - return; - } + || cmd.equals("conf") || cmd.equals("enc") + || cmd.equals("quit") || cmd.equals("bye"))) { + reply("503 Transfer in progress", false); + return; } if (!_methodDict.containsKey(cmd)) { @@ -1426,13 +1424,13 @@ private synchronized void closePassiveModeServerSocket() } @Override - public synchronized void shutdown() + public void shutdown() { super.shutdown(); /* In case of failure, we may have a transfer hanging around. */ - FtpTransfer transfer = _transfer; + FtpTransfer transfer = getTransfer(); if (transfer != null) { transfer.abort(451, "Aborting transfer due to session termination"); } @@ -1503,7 +1501,7 @@ public void messageArrived(CellMessage envelope, GFtpTransferStartedMessage message) { LOGGER.debug("Received TransferStarted message"); - FtpTransfer transfer = _transfer; + FtpTransfer transfer = getTransfer(); if (transfer != null) { transfer.transferStarted(envelope, message); } @@ -1513,7 +1511,7 @@ public void messageArrived(DoorTransferFinishedMessage message) { LOGGER.debug("Received TransferFinished message [rc={}]", message.getReturnCode()); - FtpTransfer transfer = _transfer; + FtpTransfer transfer = getTransfer(); if (transfer != null) { transfer.finished(message); } @@ -2543,11 +2541,15 @@ public void ftp_retr(String arg) } } - protected synchronized Transfer setTransfer(FtpTransfer transfer) + protected synchronized FtpTransfer getTransfer() + { + return _transfer; + } + + protected synchronized void setTransfer(FtpTransfer transfer) { _transfer = transfer; notifyAll(); - return transfer; } protected synchronized void joinTransfer() @@ -3233,7 +3235,7 @@ public void ftp_abor(String arg) { checkLoggedIn(); - FtpTransfer transfer = _transfer; + FtpTransfer transfer = getTransfer(); if (transfer != null) { transfer.abort(426, "Transfer aborted"); }