Skip to content

Commit

Permalink
Fixed issue with connection being stuck on awaitUninterruptibly() as …
Browse files Browse the repository at this point in the history
…per fizzed#1

Fixed blocking issue at channel write.
Related issue RestComm/smpp-extensions#28
  • Loading branch information
olenara committed Sep 22, 2017
1 parent 48aac99 commit 3e695e7
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 13 deletions.
20 changes: 12 additions & 8 deletions src/main/java/com/cloudhopper/smpp/impl/DefaultSmppClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -297,19 +297,23 @@ 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.option(ChannelOption.CONNECT_TIMEOUT_MILLIS, (int)connectTimeoutMillis);
// set the timeout
this.clientBootstrap.option(ChannelOption.CONNECT_TIMEOUT_MILLIS, (int)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
/* 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.
*/
logger.debug("Waiting for client connection to {}", socketAddr);
connectFuture.awaitUninterruptibly();
//assert connectFuture.isDone();
if (!connectFuture.await(connectTimeoutMillis + 1000)) {
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()) {
logger.warn("Client connection cancelled.");
Expand Down
15 changes: 10 additions & 5 deletions src/main/java/com/cloudhopper/smpp/impl/DefaultSmppSession.java
Original file line number Diff line number Diff line change
Expand Up @@ -510,16 +510,18 @@ public WindowFuture<Integer,PduRequest,PduResponse> sendRequestPdu(PduRequest pd
// write the pdu out & wait timeout amount of time
ChannelFuture channelFuture = this.channel.writeAndFlush(buffer);
if (configuration.getWriteTimeout() > 0){
channelFuture.await(configuration.getWriteTimeout());
if(!channelFuture.await(configuration.getWriteTimeout()))
throw new SmppChannelException(channelFuture.cause() != null ? channelFuture.cause().getMessage()
: "ChannelFuture failed without cause.", channelFuture.cause());
} else {
channelFuture.await();
}

// check if the write was a success
if (!channelFuture.isSuccess()) {
// the write failed, make sure to throw an exception
if (channelFuture.cause() != null) throw new SmppChannelException(channelFuture.cause().getMessage(), channelFuture.cause());
else throw new SmppChannelException("ChannelFuture failed without cause.");
throw new SmppChannelException(channelFuture.cause() != null ? channelFuture.getCause().getMessage()
: "ChannelFuture failed without cause.", channelFuture.cause());
}

this.countSendRequestPdu(pdu);
Expand Down Expand Up @@ -562,15 +564,18 @@ public void sendResponsePdu(PduResponse pdu) throws RecoverablePduException, Unr
// write the pdu out & wait timeout amount of time
ChannelFuture channelFuture = this.channel.writeAndFlush(buffer);
if(configuration.getWriteTimeout() > 0){
channelFuture.await(configuration.getWriteTimeout());
if(!channelFuture.await(configuration.getWriteTimeout()))
throw new SmppChannelException(channelFuture.cause() != null ? channelFuture.cause().getMessage()
: "ChannelFuture failed without cause.", channelFuture.cause());
} else {
channelFuture.await();
}

// check if the write was a success
if (!channelFuture.isSuccess()) {
// the write failed, make sure to throw an exception
throw new SmppChannelException(channelFuture.cause().getMessage(), channelFuture.cause());
throw new SmppChannelException(channelFuture.cause() != null ? channelFuture.cause().getMessage()
: "ChannelFuture failed without cause.", channelFuture.cause());
}
}

Expand Down

0 comments on commit 3e695e7

Please sign in to comment.