You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by sp...@apache.org on 2019/12/16 14:52:27 UTC
[tinkerpop] branch TINKERPOP-2262 created (now 93e05dc)
This is an automated email from the ASF dual-hosted git repository.
spmallette pushed a change to branch TINKERPOP-2262
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git.
at 93e05dc TINKERPOP-2262 Prevented channel close by server on protocol error
This branch includes the following new commits:
new 93e05dc TINKERPOP-2262 Prevented channel close by server on protocol error
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
[tinkerpop] 01/01: TINKERPOP-2262 Prevented channel close by server
on protocol error
Posted by sp...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
spmallette pushed a commit to branch TINKERPOP-2262
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 93e05dc1b49a6231f83b5f285e7e626996623e5f
Author: stephen <sp...@gmail.com>
AuthorDate: Mon Dec 16 09:49:29 2019 -0500
TINKERPOP-2262 Prevented channel close by server on protocol error
Allows the channel on the driver to be reused rather than replaced. Interestingly no additional error handling seemed to be needed as all tests passed.
---
CHANGELOG.asciidoc | 1 +
.../java/org/apache/tinkerpop/gremlin/driver/Connection.java | 11 +----------
.../gremlin/server/channel/WebSocketChannelizer.java | 7 ++++++-
.../tinkerpop/gremlin/server/GremlinServerIntegrateTest.java | 5 +----
4 files changed, 9 insertions(+), 15 deletions(-)
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index 0575581..b2c9c86 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -28,6 +28,7 @@ This release also includes changes from <<release-3-3-10, 3.3.10>>.
* Expanded the use of `by(String)` modulator so that it can work on `Map` as well as `Element`.
* Improved error messaging for `by(String)` so that it is more clear as to what the problem is
* Bump to Netty 4.1.42
+* Improved Gremlin Server websocket handling preventing automatic server close of the channel for protocol errors.
* Introduced internal `Buffer` API as a way to wrap Netty's Buffer API and moved `GraphBinaryReader`, `GraphBinaryWriter` and `TypeSerializer<T>` to `gremlin-core`.
* Unified the behavior of property comparison: only compare key&value.
* Supported `hasKey()` and `hasValue()` step for edge property and meta property, like `g.E().properties().hasKey('xx')`.
diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Connection.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Connection.java
index aa0868f..cdd098d 100644
--- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Connection.java
+++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Connection.java
@@ -276,17 +276,8 @@ final class Connection {
}
}
- /*
- * In the event of an IOException (typically means that the Connection might have been closed from the server side
- * - this is typical in situations like when a request is sent that exceeds maxContentLength and the server closes
- * the channel on its side) or other exceptions that indicate a non-recoverable state for the Connection object
- * (a netty CorruptedFrameException is a good example of that), the Connection cannot simply be returned to the
- * pool as future uses will end with refusal from the server and make it appear as a dead host as the write will
- * not succeed. Instead, the Connection needs to be replaced in these scenarios which destroys the dead channel
- * on the client and allows a new one to be reconstructed.
- */
private void handleConnectionCleanupOnError(final Connection thisConnection, final Throwable t) {
- if (thisConnection.isDead() || t instanceof IOException || t instanceof CodecException) {
+ if (thisConnection.isDead()) {
if (pool != null) pool.replaceConnection(thisConnection);
} else {
thisConnection.returnToPool();
diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/channel/WebSocketChannelizer.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/channel/WebSocketChannelizer.java
index 010114b..773b6fc 100644
--- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/channel/WebSocketChannelizer.java
+++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/channel/WebSocketChannelizer.java
@@ -19,6 +19,7 @@
package org.apache.tinkerpop.gremlin.server.channel;
import io.netty.handler.codec.http.websocketx.PingWebSocketFrame;
+import io.netty.handler.codec.http.websocketx.WebSocketDecoderConfig;
import org.apache.tinkerpop.gremlin.server.AbstractChannelizer;
import org.apache.tinkerpop.gremlin.server.Channelizer;
import org.apache.tinkerpop.gremlin.server.auth.AllowAllAuthenticator;
@@ -95,8 +96,12 @@ public class WebSocketChannelizer extends AbstractChannelizer {
pipeline.addLast(PIPELINE_HTTP_RESPONSE_ENCODER, new HttpResponseEncoder());
+ // setting closeOnProtocolViolation to false prevents causing all the other requests using the same channel
+ // to fail when a single request causes a protocol violation.
+ final WebSocketDecoderConfig wsDecoderConfig = WebSocketDecoderConfig.newBuilder().
+ closeOnProtocolViolation(false).allowExtensions(false).maxFramePayloadLength(settings.maxContentLength).build();
pipeline.addLast(PIPELINE_REQUEST_HANDLER, new WebSocketServerProtocolHandler(GREMLIN_ENDPOINT,
- null, false, settings.maxContentLength));
+ null, false, false, 10000L, wsDecoderConfig));
if (logger.isDebugEnabled())
pipeline.addLast(new LoggingHandler("log-aggregator-encoder", LogLevel.DEBUG));
diff --git a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java
index 5448a26..4212fac 100644
--- a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java
+++ b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java
@@ -870,10 +870,7 @@ public class GremlinServerIntegrateTest extends AbstractGremlinServerIntegration
// it seems ok to pass in this case.
} catch (Exception re) {
final Throwable root = ExceptionUtils.getRootCause(re);
- // Netty closes the channel to the server on a non-recoverable error such as CorruptedFrameException
- // and the connection is subsequently destroyed. Each of the pending requests are given an error with
- // the following error message.
- //
+
// went with two possible error messages here as i think that there is some either non-deterministic
// behavior around the error message or it's environmentally dependent (e.g. different jdk, versions, etc)
assertThat(root.getMessage(), Matchers.anyOf(is("Connection to server is no longer active"), is("Connection reset by peer")));