Skip to content

Commit

Permalink
Fix timeout math to avoid overflow
Browse files Browse the repository at this point in the history
Motivation:

In some cases we use Long.MAX_VALUE as a timeout that for all means
and purposes means infinity. This value however causes overflow when
used in expressions adding the current time with the timeout. This
overflow means that rather to never time out we time out more or less
immediately.

Modification:

Use the addWithInfinity and subWithInfinity methods that deal with
this situation.

Result:

No premature timeout for transfers.

Target: trunk
Request: 2.13
Request: 2.12
Request: 2.11
Request: 2.10
Require-notes: yes
Require-book: no
Acked-by: Paul Millar <paul.millar@desy.de>
Patch: https://rb.dcache.org/r/8490/
(cherry picked from commit 1ec6400)
  • Loading branch information
gbehrmann committed Aug 24, 2015
1 parent 60d24eb commit cc588cf
Show file tree
Hide file tree
Showing 8 changed files with 34 additions and 19 deletions.
Expand Up @@ -40,6 +40,9 @@
import org.dcache.util.Args;
import org.dcache.util.Version;

import static org.dcache.util.MathUtils.addWithInfinity;
import static org.dcache.util.MathUtils.subWithInfinity;

/**
*
*
Expand Down Expand Up @@ -632,7 +635,7 @@ public String call()
Object key = entry.getKey();
CellLock lock = entry.getValue();
sb.append(key.toString()).append(" r=");
long res = lock.getTimeout() - System.currentTimeMillis();
long res = subWithInfinity(lock.getTimeout(), System.currentTimeMillis());
sb.append(res/1000).append(" sec;");
CellMessage msg = lock.getMessage();
if (msg == null) {
Expand Down Expand Up @@ -1023,7 +1026,7 @@ public RetryingCellMessageAnswerable(CellMessage msg, CellMessageAnswerable call
this.callback = callback;
this.msg = msg;
this.executor = executor;
deadline = System.currentTimeMillis() + timeout;
deadline = addWithInfinity(System.currentTimeMillis(), timeout);
}

@Override
Expand Down Expand Up @@ -1052,7 +1055,7 @@ public void answerTimedOut(CellMessage request)

@Override
public void run() {
long timeout = deadline - System.currentTimeMillis();
long timeout = subWithInfinity(deadline, System.currentTimeMillis());
if (timeout > 0) {
sendMessage(msg, this, executor, timeout);
} else {
Expand Down
3 changes: 2 additions & 1 deletion modules/cells/src/main/java/dmg/cells/nucleus/CellLock.java
Expand Up @@ -3,6 +3,7 @@
import java.util.concurrent.Executor;

import static com.google.common.base.Preconditions.checkNotNull;
import static org.dcache.util.MathUtils.addWithInfinity;

public class CellLock
{
Expand All @@ -17,7 +18,7 @@ public CellLock(CellMessage msg, CellMessageAnswerable callback,
{
_callback = checkNotNull(callback);
_executor = checkNotNull(executor);
_timeout = System.currentTimeMillis() + timeout;
_timeout = addWithInfinity(System.currentTimeMillis(), timeout);
_message = msg;
}

Expand Down
Expand Up @@ -48,6 +48,8 @@
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.Iterables.consumingIterable;
import static com.google.common.util.concurrent.MoreExecutors.sameThreadExecutor;
import static org.dcache.util.MathUtils.addWithInfinity;
import static org.dcache.util.MathUtils.subWithInfinity;

/**
*
Expand Down Expand Up @@ -704,10 +706,10 @@ private Collection<Thread> getNonDaemonThreads(ThreadGroup group)
private boolean joinThreads(Collection<Thread> threads, long timeout)
throws InterruptedException
{
long deadline = System.currentTimeMillis() + timeout;
long deadline = addWithInfinity(System.currentTimeMillis(), timeout);
for (Thread thread: threads) {
if (thread.isAlive()) {
long wait = deadline - System.currentTimeMillis();
long wait = subWithInfinity(deadline, System.currentTimeMillis());
if (wait <= 0) {
return false;
}
Expand Down
7 changes: 5 additions & 2 deletions modules/cells/src/main/java/dmg/util/Gate.java
@@ -1,5 +1,8 @@
package dmg.util ;

import static org.dcache.util.MathUtils.addWithInfinity;
import static org.dcache.util.MathUtils.subWithInfinity;

public class Gate {
private boolean _isOpen = true ;
public Gate(){}
Expand All @@ -9,9 +12,9 @@ public Gate( boolean isOpen ){

public synchronized boolean await(long millis) throws InterruptedException
{
long deadline = System.currentTimeMillis() + millis;
long deadline = addWithInfinity(System.currentTimeMillis(), millis);
while (!_isOpen && deadline > System.currentTimeMillis()) {
wait(deadline - System.currentTimeMillis());
wait(subWithInfinity(deadline, System.currentTimeMillis()));
}
return _isOpen;
}
Expand Down
@@ -1,20 +1,20 @@
package org.dcache.util;

public class MathUtils {

public class MathUtils
{
/**
* Return the absolute value of an integer, modulo some other integer.
* This is similar to the naive {@code Math.abs(value) % modulo} except it handles
* the case when value is Integer.MIN_VALUE correctly.
*/
static public int absModulo(int value, int modulo)
public static int absModulo(int value, int modulo)
{
return Math.abs(value % modulo);
}

/**
* Implements long addition, treating MAX_VALUE and MIN_VALUE as positive
* and negative infinity respecitvely. Semantics are similar to Double
* and negative infinity respectively. Semantics are similar to Double
* arithmetic. Overflow results in positive infinity, underflow in
* negative infinity.
*
Expand Down Expand Up @@ -48,7 +48,7 @@ public static long addWithInfinity(long a, long b)

/**
* Implements long subtraction, treating MAX_VALUE and MIN_VALUE as
* positive and negative infinity respecitvely. Semantics are similar
* positive and negative infinity respectively. Semantics are similar
* to Double arithmetic. Overflow results in positive infinity, underflow
* in negative infinity.
*
Expand Down
Expand Up @@ -24,6 +24,9 @@
import org.dcache.util.Args;
import org.dcache.util.Transfer;

import static org.dcache.util.MathUtils.addWithInfinity;
import static org.dcache.util.MathUtils.subWithInfinity;

/**
* @author Patrick Fuhrmann
* @version 1.0, Aug 04 2001
Expand Down Expand Up @@ -271,9 +274,9 @@ private synchronized void _stateChanged( int event ){
}
private synchronized void waitForFinish( long timeout )
throws InterruptedException {
long end = System.currentTimeMillis() + timeout ;
long end = addWithInfinity(System.currentTimeMillis(), timeout);
while( _state != __WeAreFinished ){
long rest = end - System.currentTimeMillis() ;
long rest = subWithInfinity(end, System.currentTimeMillis());
_log.info( "waitForFinish : waiting for "+rest+" seconds" ) ;
if( rest <=0 ) {
break;
Expand Down
Expand Up @@ -9,6 +9,9 @@
import diskCacheV111.util.PnfsHandler;
import diskCacheV111.util.TimeoutCacheException;

import static org.dcache.util.MathUtils.addWithInfinity;
import static org.dcache.util.MathUtils.subWithInfinity;

/**
* A transfer where the mover can send a redirect message to the door.
*/
Expand Down Expand Up @@ -63,10 +66,10 @@ public synchronized T waitForRedirect(long millis)
try {
setStatus("Mover " + getPool() + "/" +
getMoverId() + ": Waiting for redirect");
long deadline = System.currentTimeMillis() + millis;
long deadline = addWithInfinity(System.currentTimeMillis(), millis);
while (hasMover() && !_isRedirected &&
System.currentTimeMillis() < deadline) {
wait(deadline - System.currentTimeMillis());
wait(subWithInfinity(deadline, System.currentTimeMillis()));
}

if (waitForMover(0)) {
Expand Down
4 changes: 2 additions & 2 deletions modules/dcache/src/main/java/org/dcache/util/Transfer.java
Expand Up @@ -552,9 +552,9 @@ public boolean waitForMover(long timeout, TimeUnit unit)
public synchronized boolean waitForMover(long millis)
throws CacheException, InterruptedException
{
long deadline = System.currentTimeMillis() + millis;
long deadline = addWithInfinity(System.currentTimeMillis(), millis);
while (!_hasMoverFinished && System.currentTimeMillis() < deadline) {
wait(deadline - System.currentTimeMillis());
wait(subWithInfinity(deadline, System.currentTimeMillis()));
}

if (_error != null) {
Expand Down

0 comments on commit cc588cf

Please sign in to comment.