Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding workaround for the case when a connect gets stuck forever #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Adding workaround for the case when a connect gets stuck forever #1

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
43a45e9
Adding workaround for the case when a connect gets stuck forever
elruwen Jan 4, 2016
8c434eb
Merge remote-tracking branch 'origin/master' into deadlock-on-connect…
elruwen Jan 5, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 39 additions & 30 deletions src/main/java/com/cloudhopper/smpp/impl/DefaultSmppClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,17 @@
* #L%
*/

import com.cloudhopper.smpp.SmppClient;
import com.cloudhopper.smpp.util.DaemonExecutors;
import com.cloudhopper.smpp.SmppBindType;
import com.cloudhopper.smpp.type.SmppChannelException;
import com.cloudhopper.smpp.SmppClient;
import com.cloudhopper.smpp.SmppSession;
import com.cloudhopper.smpp.SmppSessionConfiguration;
import com.cloudhopper.smpp.SmppSessionHandler;
import com.cloudhopper.smpp.channel.SmppChannelConstants;
import com.cloudhopper.smpp.type.SmppTimeoutException;
import com.cloudhopper.smpp.channel.SmppClientConnector;
import com.cloudhopper.smpp.channel.SmppSessionPduDecoder;
import com.cloudhopper.smpp.channel.SmppSessionLogger;
import com.cloudhopper.smpp.channel.SmppSessionWrapper;
import com.cloudhopper.smpp.channel.SmppSessionPduDecoder;
import com.cloudhopper.smpp.channel.SmppSessionThreadRenamer;
import com.cloudhopper.smpp.channel.SmppSessionWrapper;
import com.cloudhopper.smpp.pdu.BaseBind;
import com.cloudhopper.smpp.pdu.BaseBindResp;
import com.cloudhopper.smpp.pdu.BindReceiver;
Expand All @@ -45,12 +42,10 @@
import com.cloudhopper.smpp.type.SmppBindException;
import com.cloudhopper.smpp.type.SmppChannelConnectException;
import com.cloudhopper.smpp.type.SmppChannelConnectTimeoutException;
import com.cloudhopper.smpp.type.SmppChannelException;
import com.cloudhopper.smpp.type.SmppTimeoutException;
import com.cloudhopper.smpp.type.UnrecoverablePduException;
import java.net.InetSocketAddress;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import javax.net.ssl.SSLEngine;
import com.cloudhopper.smpp.util.DaemonExecutors;
import org.jboss.netty.bootstrap.ClientBootstrap;
import org.jboss.netty.channel.Channel;
import org.jboss.netty.channel.ChannelFuture;
Expand All @@ -63,6 +58,12 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.net.ssl.SSLEngine;
import java.net.InetSocketAddress;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;

/**
* Default implementation to "bootstrap" client SMPP sessions (create & bind).
*
Expand Down Expand Up @@ -273,28 +274,36 @@ protected Channel createConnectedChannel(String host, int port, long connectTime
// a socket address used to "bind" to the remote system
InetSocketAddress socketAddr = new InetSocketAddress(host, port);

// set the timeout
this.clientBootstrap.setOption("connectTimeoutMillis", connectTimeoutMillis);
// set the timeout
this.clientBootstrap.setOption("connectTimeoutMillis", connectTimeoutMillis);

// attempt to connect to the remote system
ChannelFuture connectFuture = this.clientBootstrap.connect(socketAddr);

// wait until the connection is made successfully
// boolean timeout = !connectFuture.await(connectTimeoutMillis);
// BAD: using .await(timeout)
// see http://netty.io/3.9/api/org/jboss/netty/channel/ChannelFuture.html
connectFuture.awaitUninterruptibly();
//assert connectFuture.isDone();

if (connectFuture.isCancelled()) {
throw new InterruptedException("connectFuture cancelled by user");
} else if (!connectFuture.isSuccess()) {
if (connectFuture.getCause() instanceof org.jboss.netty.channel.ConnectTimeoutException) {
throw new SmppChannelConnectTimeoutException("Unable to connect to host [" + host + "] and port [" + port + "] within " + connectTimeoutMillis + " ms", connectFuture.getCause());
} else {
throw new SmppChannelConnectException("Unable to connect to host [" + host + "] and port [" + port + "]: " + connectFuture.getCause().getMessage(), connectFuture.getCause());
}
}

// Wait until the connection is made successfully.
// According to the netty documentation it is bad to use .await(timeout). Instead
// b.setOption("connectTimeoutMillis", 10000);
// should be used. See http://netty.io/3.9/api/org/jboss/netty/channel/ChannelFuture.html
// It turns out that under certain unknown circumstances the connect waits forever: https://github.com/twitter/cloudhopper-smpp/issues/117
// That's why the future is canceled 1 second after the specified timeout.
// This is a workaround and hopefully not needed after the switch to netty 4.
if (!connectFuture.await(connectTimeoutMillis + 1000)) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally used two minutes just to be on the safe side, but I think 1 second should do the trick. I don't want to use the same time as the real timeout in order not to interfere with the real timeout.

logger.error("connectFuture did not finish in expected time! Try to cancel the connectFuture");
boolean isCanceled = connectFuture.cancel();
logger.error("connectFuture: isCanceled {} isDone {} isSuccess {}", isCanceled, connectFuture.isDone(), connectFuture.isSuccess());
throw new SmppChannelConnectTimeoutException("Could not connect to the server within timeout");
}


if (connectFuture.isCancelled()) {
throw new InterruptedException("connectFuture cancelled by user");
} else if (!connectFuture.isSuccess()) {
if (connectFuture.getCause() instanceof org.jboss.netty.channel.ConnectTimeoutException) {
throw new SmppChannelConnectTimeoutException("Unable to connect to host [" + host + "] and port [" + port + "] within " + connectTimeoutMillis + " ms", connectFuture.getCause());
} else {
throw new SmppChannelConnectException("Unable to connect to host [" + host + "] and port [" + port + "]: " + connectFuture.getCause().getMessage(), connectFuture.getCause());
}
}

// if we get here, then we were able to connect and get a channel
return connectFuture.getChannel();
Expand Down