THRIFT-4805: Suppress excessive logging of SASL TTransportExceptions in case of END_OF_FILE
Two fixes here:
1. Additional logic to properly catch and handle TTransportException.
Currently, T(SASL)TransportException gets caught and handled in
the wrong catch-block.
2. The fix for THRIFT-3769 mutes _all_ TTransportExceptions in TThreadPoolServer.
This might mute legitimate failures. The intent of THRIFT-3769 (and
THRIFT-2268) was to mute the noise caused by TTransportException.END_OF_FILE.
This commit lets legitimate failures to be bubbled upwards.
diff --git a/CHANGES.md b/CHANGES.md
index df73121..9f2d1e4 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -25,6 +25,7 @@
- [THRIFT-4709](https://issues.apache.org/jira/browse/THRIFT-4709) - java: changes to UTF-8 handling require JDK 1.7 at a minimum
- [THRIFT-4712](https://issues.apache.org/jira/browse/THRIFT-4712) - java: class org.apache.thrift.ShortStack is no longer public
- [THRIFT-4725](https://issues.apache.org/jira/browse/THRIFT-4725) - java: change return type signature of 'process' methods
+- [THRIFT-4805](https://issues.apache.org/jira/browse/THRIFT-4805) - java: Suppress excessive logging of SASL TTransportExceptions in case of END_OF_FILE
- [THRIFT-4675](https://issues.apache.org/jira/browse/THRIFT-4675) - js: now uses node-int64 for 64 bit integer constants
- [THRIFT-4841](https://issues.apache.org/jira/browse/THRIFT-4841) - delphi: old THTTPTransport is now TMsxmlHTTPTransport
- [THRIFT-4536](https://issues.apache.org/jira/browse/THRIFT-4536) - rust: convert from try-from crate to rust stable (1.34+), re-export ordered-float
diff --git a/lib/java/README.md b/lib/java/README.md
index 7dca456..78e6445 100644
--- a/lib/java/README.md
+++ b/lib/java/README.md
@@ -170,9 +170,12 @@
## 1.0
-The signature of the 'process' method in TAsyncProcessor and TProcessor has
+* The signature of the 'process' method in TAsyncProcessor and TProcessor has
changed to remove a boolean return type and to instead rely on Exceptions.
+* Per THRIFT-4805, TSaslTransportException has been removed. The same condition
+is now covered by TTansportException, where `TTransportException.getType() == END_OF_FILE`.
+
## 0.12.0
The access modifier of the AutoExpandingBuffer class has been changed from
diff --git a/lib/java/src/org/apache/thrift/server/TThreadPoolServer.java b/lib/java/src/org/apache/thrift/server/TThreadPoolServer.java
index db1ecb9..87e8733 100644
--- a/lib/java/src/org/apache/thrift/server/TThreadPoolServer.java
+++ b/lib/java/src/org/apache/thrift/server/TThreadPoolServer.java
@@ -19,6 +19,8 @@
package org.apache.thrift.server;
+import java.util.Arrays;
+import java.util.List;
import java.util.Random;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.RejectedExecutionException;
@@ -29,14 +31,12 @@
import org.apache.thrift.TException;
import org.apache.thrift.TProcessor;
import org.apache.thrift.protocol.TProtocol;
-import org.apache.thrift.transport.TSaslTransportException;
import org.apache.thrift.transport.TServerTransport;
import org.apache.thrift.transport.TTransport;
import org.apache.thrift.transport.TTransportException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-
/**
* Server which uses Java's built in ThreadPool management to spawn off
* a worker pool that
@@ -312,21 +312,13 @@
}
processor.process(inputProtocol, outputProtocol);
}
- } catch (TException tx) {
- LOGGER.error("Thrift error occurred during processing of message.", tx);
} catch (Exception x) {
// We'll usually receive RuntimeException types here
// Need to unwrap to ascertain real causing exception before we choose to ignore
- Throwable realCause = x.getCause();
// Ignore err-logging all transport-level/type exceptions
- if ((realCause != null && realCause instanceof TTransportException)
- || (x instanceof TTransportException)) {
- LOGGER.debug(
- "Received TTransportException during processing of message. Ignoring.",
- x);
- } else {
+ if (!isIgnorableException(x)) {
// Log the exception at error level and continue
- LOGGER.error("Error occurred during processing of message.", x);
+ LOGGER.error((x instanceof TException? "Thrift " : "") + "Error occurred during processing of message.", x);
}
} finally {
if (eventHandler != null) {
@@ -343,5 +335,25 @@
}
}
}
+
+ private boolean isIgnorableException(Exception x) {
+ TTransportException tTransportException = null;
+
+ if (x instanceof TTransportException) {
+ tTransportException = (TTransportException)x;
+ }
+ else if (x.getCause() instanceof TTransportException) {
+ tTransportException = (TTransportException)x.getCause();
+ }
+
+ if (tTransportException != null) {
+ switch(tTransportException.getType()) {
+ case TTransportException.END_OF_FILE:
+ case TTransportException.TIMED_OUT:
+ return true;
+ }
+ }
+ return false;
+ }
}
}
diff --git a/lib/java/src/org/apache/thrift/transport/TSaslTransport.java b/lib/java/src/org/apache/thrift/transport/TSaslTransport.java
index c858425..4a453b6 100644
--- a/lib/java/src/org/apache/thrift/transport/TSaslTransport.java
+++ b/lib/java/src/org/apache/thrift/transport/TSaslTransport.java
@@ -310,14 +310,11 @@
underlyingTransport.close();
}
} catch (TTransportException e) {
- /*
- * If there is no-data or no-sasl header in the stream, throw a different
- * type of exception so we can handle this scenario differently.
- */
+ // If there is no-data or no-sasl header in the stream,
+ // log the failure, and clean up the underlying transport.
if (!readSaslHeader && e.getType() == TTransportException.END_OF_FILE) {
underlyingTransport.close();
- LOGGER.debug("No data or no sasl data in the stream");
- throw new TSaslTransportException("No data or no sasl data in the stream during negotiation", e);
+ LOGGER.debug("No data or no sasl data in the stream during negotiation");
}
throw e;
}
@@ -427,6 +424,14 @@
readFrame();
} catch (SaslException e) {
throw new TTransportException(e);
+ } catch (TTransportException transportException) {
+ // If there is no-data or no-sasl header in the stream, log the failure, and rethrow.
+ if (transportException.getType() == TTransportException.END_OF_FILE) {
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug("No data or no sasl data in the stream during negotiation");
+ }
+ }
+ throw transportException;
}
return readBuffer.read(buf, off, len);
diff --git a/lib/java/src/org/apache/thrift/transport/TSaslTransportException.java b/lib/java/src/org/apache/thrift/transport/TSaslTransportException.java
deleted file mode 100644
index 90189f4..0000000
--- a/lib/java/src/org/apache/thrift/transport/TSaslTransportException.java
+++ /dev/null
@@ -1,43 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-
-package org.apache.thrift.transport;
-
-/*
- * This exception is used to track exceptions in TSaslTransport
- * that does not have Sasl signature in their stream.
- */
-public class TSaslTransportException extends TTransportException {
-
- public TSaslTransportException() {
- super();
- }
-
- public TSaslTransportException(String message) {
- super(message);
- }
-
- public TSaslTransportException(Throwable cause) {
- super(cause);
- }
-
- public TSaslTransportException(String message, Throwable cause) {
- super(message, cause);
- }
-}