This is an automated email from the ASF dual-hosted git repository. jking pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/thrift.git
commit c35ed736d26a1dfd8965ae197a67904ed9b4fba3 Author: Mithun RK <[email protected]> AuthorDate: Mon Mar 11 14:14:05 2019 -0700 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. --- CHANGES.md | 1 + lib/java/README.md | 5 ++- .../apache/thrift/server/TThreadPoolServer.java | 36 ++++++++++++------ .../apache/thrift/transport/TSaslTransport.java | 17 ++++++--- .../thrift/transport/TSaslTransportException.java | 43 ---------------------- 5 files changed, 40 insertions(+), 62 deletions(-) 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 @@ http://gradle.org/ ## 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 java.util.concurrent.TimeUnit; 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 @@ public class TThreadPoolServer extends TServer { } 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 @@ public class TThreadPoolServer extends TServer { } } } + + 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 @@ abstract class TSaslTransport extends TTransport { 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 @@ abstract class TSaslTransport extends TTransport { 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); - } -}
