Skip to content

Commit 6be5875

Browse files
committed
Added debug logging as per ticket 34388.
Fixed issue with connection being stuck on awaitUninterruptibly() as per fizzed#1 Fixed blocking issue at channel write. Related issue RestComm/smpp-extensions#28
1 parent 4b8702b commit 6be5875

File tree

2 files changed

+77
-45
lines changed

2 files changed

+77
-45
lines changed

src/main/java/com/cloudhopper/smpp/impl/DefaultSmppClient.java

Lines changed: 62 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -190,15 +190,25 @@ public SmppSession bind(SmppSessionConfiguration config, SmppSessionHandler sess
190190
DefaultSmppSession session = null;
191191
try {
192192
// connect to the remote system and create the session
193+
logger.debug("Connecting to remote system " + config.getName() + " host " + config.getHost() + ":" + config.getPort());
193194
session = doOpen(config, sessionHandler);
194195

195196
// try to bind to the remote system (may throw an exception)
197+
logger.debug("Binding to remote system " + config.getName());
196198
doBind(session, config, sessionHandler);
199+
200+
logger.debug("Successfully bound to " + config.getName());
197201
} finally {
198202
// close the session if we weren't able to bind correctly
199203
if (session != null && !session.isBound()) {
200204
// make sure that the resources are always cleaned up
201-
try { session.close(); } catch (Exception e) { }
205+
try {
206+
logger.debug("Closing session - not able to bind to " + config.getName());
207+
session.close();
208+
}
209+
catch (Exception e) {
210+
logger.debug("Exception while trying to close connection to " + config.getName(), e);
211+
}
202212
}
203213
}
204214
return session;
@@ -212,7 +222,9 @@ protected void doBind(DefaultSmppSession session, SmppSessionConfiguration confi
212222
try {
213223
// attempt to bind to the SMSC
214224
// session implementation handles error checking, version negotiation, and can be discarded
225+
215226
bindResp = session.bind(bindRequest, config.getBindTimeout());
227+
logger.debug("Bound to esme systemId=" + config.getSystemId());
216228
} catch (RecoverablePduException e) {
217229
// if a bind fails, there really is no recovery...
218230
throw new UnrecoverablePduException(e.getMessage(), e);
@@ -229,19 +241,20 @@ protected DefaultSmppSession doOpen(SmppSessionConfiguration config, SmppSession
229241
protected DefaultSmppSession createSession(Channel channel, SmppSessionConfiguration config, SmppSessionHandler sessionHandler) throws SmppTimeoutException, SmppChannelException, InterruptedException {
230242
DefaultSmppSession session = new DefaultSmppSession(SmppSession.Type.CLIENT, config, channel, sessionHandler, monitorExecutor);
231243

232-
// add SSL handler
244+
logger.debug("Creating session with esme " + config.getSystemId());
245+
// add SSL handler
233246
if (config.isUseSsl()) {
234-
SslConfiguration sslConfig = config.getSslConfiguration();
235-
if (sslConfig == null) throw new IllegalStateException("sslConfiguration must be set");
236-
try {
237-
SslContextFactory factory = new SslContextFactory(sslConfig);
238-
SSLEngine sslEngine = factory.newSslEngine();
239-
sslEngine.setUseClientMode(true);
240-
channel.getPipeline().addLast(SmppChannelConstants.PIPELINE_SESSION_SSL_NAME, new SslHandler(sslEngine));
241-
} catch (Exception e) {
242-
throw new SmppChannelConnectException("Unable to create SSL session]: " + e.getMessage(), e);
243-
}
244-
}
247+
SslConfiguration sslConfig = config.getSslConfiguration();
248+
if (sslConfig == null) throw new IllegalStateException("sslConfiguration must be set");
249+
try {
250+
SslContextFactory factory = new SslContextFactory(sslConfig);
251+
SSLEngine sslEngine = factory.newSslEngine();
252+
sslEngine.setUseClientMode(true);
253+
channel.getPipeline().addLast(SmppChannelConstants.PIPELINE_SESSION_SSL_NAME, new SslHandler(sslEngine));
254+
} catch (Exception e) {
255+
throw new SmppChannelConnectException("Unable to create SSL session]: " + e.getMessage(), e);
256+
}
257+
}
245258

246259
// add the thread renamer portion to the pipeline
247260
if (config.getName() != null) {
@@ -254,11 +267,11 @@ protected DefaultSmppSession createSession(Channel channel, SmppSessionConfigura
254267
SmppSessionLogger loggingHandler = new SmppSessionLogger(DefaultSmppSession.class.getCanonicalName(), config.getLoggingOptions());
255268
channel.getPipeline().addLast(SmppChannelConstants.PIPELINE_SESSION_LOGGER_NAME, loggingHandler);
256269

257-
// add a writeTimeout handler after the logger
258-
if (config.getWriteTimeout() > 0) {
259-
WriteTimeoutHandler writeTimeoutHandler = new WriteTimeoutHandler(writeTimeoutTimer, config.getWriteTimeout(), TimeUnit.MILLISECONDS);
260-
channel.getPipeline().addLast(SmppChannelConstants.PIPELINE_SESSION_WRITE_TIMEOUT_NAME, writeTimeoutHandler);
261-
}
270+
// add a writeTimeout handler after the logger
271+
if (config.getWriteTimeout() > 0) {
272+
WriteTimeoutHandler writeTimeoutHandler = new WriteTimeoutHandler(writeTimeoutTimer, config.getWriteTimeout(), TimeUnit.MILLISECONDS);
273+
channel.getPipeline().addLast(SmppChannelConstants.PIPELINE_SESSION_WRITE_TIMEOUT_NAME, writeTimeoutHandler);
274+
}
262275

263276
// add a new instance of a decoder (that takes care of handling frames)
264277
channel.getPipeline().addLast(SmppChannelConstants.PIPELINE_SESSION_PDU_DECODER_NAME, new SmppSessionPduDecoder(session.getTranscoder()));
@@ -275,29 +288,39 @@ protected Channel createConnectedChannel(String host, int port, long connectTime
275288

276289
// set the timeout
277290
this.clientBootstrap.setOption("connectTimeoutMillis", connectTimeoutMillis);
291+
292+
// attempt to connect to the remote system
293+
logger.debug("Attempting to connect to remote system " + host + ":" + port);
294+
ChannelFuture connectFuture = this.clientBootstrap.connect(socketAddr);
278295

279-
// attempt to connect to the remote system
280-
ChannelFuture connectFuture = this.clientBootstrap.connect(socketAddr);
281-
282-
// wait until the connection is made successfully
283-
// boolean timeout = !connectFuture.await(connectTimeoutMillis);
284-
// BAD: using .await(timeout)
285-
// see http://netty.io/3.9/api/org/jboss/netty/channel/ChannelFuture.html
286-
connectFuture.awaitUninterruptibly();
287-
//assert connectFuture.isDone();
288-
289-
if (connectFuture.isCancelled()) {
290-
throw new InterruptedException("connectFuture cancelled by user");
291-
} else if (!connectFuture.isSuccess()) {
292-
if (connectFuture.getCause() instanceof org.jboss.netty.channel.ConnectTimeoutException) {
293-
throw new SmppChannelConnectTimeoutException("Unable to connect to host [" + host + "] and port [" + port + "] within " + connectTimeoutMillis + " ms", connectFuture.getCause());
294-
} else {
295-
throw new SmppChannelConnectException("Unable to connect to host [" + host + "] and port [" + port + "]: " + connectFuture.getCause().getMessage(), connectFuture.getCause());
296-
}
297-
}
296+
/* Wait until the connection is made successfully.
297+
* According to the netty documentation it is bad to use .await(timeout). Instead
298+
* b.setOption("connectTimeoutMillis", 10000);
299+
* should be used. See http://netty.io/3.9/api/org/jboss/netty/channel/ChannelFuture.html
300+
* It turns out that under certain unknown circumstances the connect waits forever: https://github.com/twitter/cloudhopper-smpp/issues/117
301+
* That's why the future is canceled 1 second after the specified timeout.
302+
* This is a workaround and hopefully not needed after the switch to netty 4.
303+
*/
304+
if (!connectFuture.await(connectTimeoutMillis + 1000)) {
305+
logger.error("connectFuture did not finish in expected time! Try to cancel the connectFuture");
306+
boolean isCanceled = connectFuture.cancel();
307+
logger.error("connectFuture: isCanceled {} isDone {} isSuccess {}", isCanceled, connectFuture.isDone(), connectFuture.isSuccess());
308+
throw new SmppChannelConnectTimeoutException("Could not connect to the server within timeout");
309+
}
298310

299-
// if we get here, then we were able to connect and get a channel
300-
return connectFuture.getChannel();
311+
if (connectFuture.isCancelled()) {
312+
throw new InterruptedException("connectFuture cancelled by user");
313+
} else if (!connectFuture.isSuccess()) {
314+
if (connectFuture.getCause() instanceof org.jboss.netty.channel.ConnectTimeoutException) {
315+
throw new SmppChannelConnectTimeoutException("Unable to connect to host [" + host + "] and port [" + port + "] within " + connectTimeoutMillis + " ms", connectFuture.getCause());
316+
} else {
317+
throw new SmppChannelConnectException("Unable to connect to host [" + host + "] and port [" + port + "]: " + connectFuture.getCause().getMessage(), connectFuture.getCause());
318+
}
319+
}
320+
321+
logger.debug("Successfully connected to remote system " + host + ":" + port);
322+
// if we get here, then we were able to connect and get a channel
323+
return connectFuture.getChannel();
301324
}
302325

303326
}

src/main/java/com/cloudhopper/smpp/impl/DefaultSmppSession.java

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,6 @@ protected BaseBindResp bind(BaseBind request, long timeoutInMillis) throws Recov
340340
// bind failed for a specific reason
341341
throw new SmppBindException(bindResponse);
342342
}
343-
344343
// if we make it all the way here, we're good and bound
345344
bound = true;
346345

@@ -373,7 +372,12 @@ protected BaseBindResp bind(BaseBind request, long timeoutInMillis) throws Recov
373372
setBound();
374373
} else {
375374
// the bind failed, we need to clean up resources
376-
try { this.close(); } catch (Exception e) { }
375+
try {
376+
logger.debug("Bound failed for systemId=" + request.getSystemId());
377+
this.close();
378+
} catch (Exception e) {
379+
logger.debug("Exception after bind failed for systemId=" + request.getSystemId());
380+
}
377381
}
378382
}
379383
}
@@ -527,8 +531,10 @@ public WindowFuture<Integer,PduRequest,PduResponse> sendRequestPdu(PduRequest pd
527531
}
528532

529533
// write the pdu out & wait timeout amount of time
530-
ChannelFuture channelFuture = this.channel.write(buffer).await();
531-
534+
ChannelFuture channelFuture = this.channel.write(buffer);
535+
if(!channelFuture.await(timeoutMillis))
536+
throw new SmppChannelException(channelFuture.getCause().getMessage(), channelFuture.getCause());
537+
532538
// check if the write was a success
533539
if (!channelFuture.isSuccess()) {
534540
// the write failed, make sure to throw an exception
@@ -573,8 +579,11 @@ public void sendResponsePdu(PduResponse pdu) throws RecoverablePduException, Unr
573579
}
574580

575581
// write the pdu out & wait timeout amount of time
576-
ChannelFuture channelFuture = this.channel.write(buffer).await();
577-
582+
ChannelFuture channelFuture = this.channel.write(buffer);
583+
//REQUESTED BY SERGEI VETUNIEV FOR TESTING
584+
if(!channelFuture.await(10000))
585+
throw new SmppChannelException(channelFuture.getCause().getMessage(), channelFuture.getCause());
586+
578587
// check if the write was a success
579588
if (!channelFuture.isSuccess()) {
580589
// the write failed, make sure to throw an exception

0 commit comments

Comments
 (0)