[
https://issues.apache.org/jira/browse/TINKERPOP-2717?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17507457#comment-17507457
]
ASF GitHub Bot commented on TINKERPOP-2717:
-------------------------------------------
FlorianHockmann commented on a change in pull request #1584:
URL: https://github.com/apache/tinkerpop/pull/1584#discussion_r827751866
##########
File path: gremlin-dotnet/test/Gremlin.Net.UnitTest/Driver/ConnectionTests.cs
##########
@@ -0,0 +1,204 @@
+#region License
+
+/*
+ * 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.
+ */
+
+#endregion
+
+using System;
+using System.Net.WebSockets;
+using System.Threading;
+using System.Threading.Tasks;
+using Gremlin.Net.Driver;
+using Gremlin.Net.Driver.Exceptions;
+using Gremlin.Net.Driver.Messages;
+using Gremlin.Net.Structure.IO.GraphSON;
+using Moq;
+using Xunit;
+
+namespace Gremlin.Net.UnitTest.Driver
+{
+ public class ConnectionTests
+ {
+ [Fact]
+ public async Task ShouldHandleCloseMessageAfterConnectAsync()
+ {
+ var mockedConnectionFactory = new Mock<IConnectionFactory>();
Review comment:
(nitpick) This is unused.
##########
File path: gremlin-dotnet/test/Gremlin.Net.UnitTest/Driver/ConnectionTests.cs
##########
@@ -0,0 +1,204 @@
+#region License
+
+/*
+ * 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.
+ */
+
+#endregion
+
+using System;
+using System.Net.WebSockets;
+using System.Threading;
+using System.Threading.Tasks;
+using Gremlin.Net.Driver;
+using Gremlin.Net.Driver.Exceptions;
+using Gremlin.Net.Driver.Messages;
+using Gremlin.Net.Structure.IO.GraphSON;
+using Moq;
+using Xunit;
+
+namespace Gremlin.Net.UnitTest.Driver
+{
+ public class ConnectionTests
+ {
+ [Fact]
+ public async Task ShouldHandleCloseMessageAfterConnectAsync()
+ {
+ var mockedConnectionFactory = new Mock<IConnectionFactory>();
+ var mockedClientWebSocket = new Mock<IClientWebSocket>();
+ mockedClientWebSocket
+ .Setup(m => m.ReceiveAsync(It.IsAny<ArraySegment<byte>>(),
CancellationToken.None))
Review comment:
(nitpick) Really not important, but you could replace these
`CancellationToken.None` with `It.IsAny<CancellationToken>()` as we might use
the token here in the future and that shouldn't break this test.
##########
File path: gremlin-dotnet/src/Gremlin.Net/Driver/WebSocketConnection.cs
##########
@@ -128,5 +135,61 @@ protected virtual void Dispose(bool disposing)
}
#endregion
+
+ private static IClientWebSocket
CreateClientWebSocket(WebSocketSettings settings)
+ {
+ if (settings.WebSocketFactoryCallback != null)
+ {
+ return settings.WebSocketFactoryCallback(settings);
+ }
+
+ return new ProxyClientWebSocket(new ClientWebSocket());
+ }
+
+ private class ProxyClientWebSocket : IClientWebSocket
+ {
+ private readonly ClientWebSocket client;
+
+ public ProxyClientWebSocket(ClientWebSocket client)
+ {
+ this.client = client;
+ }
+
+ public WebSocketState State => this.client.State;
+
+ public ClientWebSocketOptions Options => this.client.Options;
+
+ public Task CloseAsync(WebSocketCloseStatus closeStatus, string
statusDescription, CancellationToken cancellationToken)
+ {
+ return this.client.CloseAsync(closeStatus, statusDescription,
cancellationToken);
Review comment:
(nitpick) Again, this is not important, but you could remove all these
`this.`
##########
File path: gremlin-dotnet/test/Gremlin.Net.UnitTest/Driver/ConnectionTests.cs
##########
@@ -0,0 +1,204 @@
+#region License
+
+/*
+ * 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.
+ */
+
+#endregion
+
+using System;
+using System.Net.WebSockets;
+using System.Threading;
+using System.Threading.Tasks;
+using Gremlin.Net.Driver;
+using Gremlin.Net.Driver.Exceptions;
+using Gremlin.Net.Driver.Messages;
+using Gremlin.Net.Structure.IO.GraphSON;
+using Moq;
+using Xunit;
+
+namespace Gremlin.Net.UnitTest.Driver
+{
+ public class ConnectionTests
+ {
+ [Fact]
+ public async Task ShouldHandleCloseMessageAfterConnectAsync()
+ {
+ var mockedConnectionFactory = new Mock<IConnectionFactory>();
+ var mockedClientWebSocket = new Mock<IClientWebSocket>();
+ mockedClientWebSocket
+ .Setup(m => m.ReceiveAsync(It.IsAny<ArraySegment<byte>>(),
CancellationToken.None))
+ .ReturnsAsync(new WebSocketReceiveResult(0,
WebSocketMessageType.Close, true, WebSocketCloseStatus.MessageTooBig, "Message
is too large"));
+ mockedClientWebSocket
+ .SetupGet(m => m.Options).Returns(new
ClientWebSocket().Options);
+
+ Uri uri = new Uri("wss://localhost:8182");
+ Connection connection = GetConnection(mockedClientWebSocket, uri);
+
+ await connection.ConnectAsync(CancellationToken.None);
+
+ Assert.False(connection.IsOpen);
+ Assert.Equal(0, connection.NrRequestsInFlight);
+ mockedClientWebSocket.Verify(m => m.ConnectAsync(uri,
CancellationToken.None), Times.Once);
+ mockedClientWebSocket.Verify(m =>
m.ReceiveAsync(It.IsAny<ArraySegment<byte>>(), CancellationToken.None),
Times.Once);
+ mockedClientWebSocket.Verify(m =>
m.CloseAsync(WebSocketCloseStatus.NormalClosure, string.Empty,
CancellationToken.None), Times.Once);
+ }
+
+ [Fact]
+ public async Task
ShouldThrowIfClosedMessageRecievedWithValidPropertiesAsync()
Review comment:
typo: _Rec**ei**ved_
##########
File path: gremlin-dotnet/src/Gremlin.Net/Driver/IClientWebSocket.cs
##########
@@ -0,0 +1,43 @@
+#region License
+
+/*
+ * 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.
+ */
+
+#endregion
+
+using System;
+using System.Net.WebSockets;
+using System.Threading;
+using System.Threading.Tasks;
+
+namespace Gremlin.Net.Driver
+{
+ interface IClientWebSocket : IDisposable
Review comment:
(nitpick) I would add the `internal` modifier here. It's implicit and
therefore not really necessary, but I think it makes it easier to read this way
as it makes the modifier obvious.
##########
File path: gremlin-dotnet/src/Gremlin.Net/Driver/WebSocketConnection.cs
##########
@@ -128,5 +135,61 @@ protected virtual void Dispose(bool disposing)
}
#endregion
+
+ private static IClientWebSocket
CreateClientWebSocket(WebSocketSettings settings)
+ {
+ if (settings.WebSocketFactoryCallback != null)
Review comment:
I don't think this should be part of the settings as those should really
only contain configuration values in my opinion. My suggestion is instead to
add the `IClientWebSocket` as a constructor parameter to the
`WebSocketConnection` class and also to the `Connection` class and then set it
in the `ConnectionFactory` where the `Connection` is instantiated.
That still makes it easy to use a mocked `IClientWebSocket` in tests and it
also clearly shows the dependencies of the `WebSocketConnection` class.
An alternative would be to move the `WebSocketFactoryCallback` property out
of the settings directly into the `WebSocketConnection` class. This would
however require to move the instantiation of the `ClientWebSocket` out of the
constructor into the `ConnectAsync()` method.
I would then also initialize the property with the default value `() => new
ProxyClientWebSocket(new ClientWebSocket())` and only change it in tests. Then
you could drop this null check here.
##########
File path: gremlin-dotnet/src/Gremlin.Net/Driver/WebSocketConnection.cs
##########
@@ -128,5 +135,61 @@ protected virtual void Dispose(bool disposing)
}
#endregion
+
+ private static IClientWebSocket
CreateClientWebSocket(WebSocketSettings settings)
+ {
+ if (settings.WebSocketFactoryCallback != null)
+ {
+ return settings.WebSocketFactoryCallback(settings);
+ }
+
+ return new ProxyClientWebSocket(new ClientWebSocket());
+ }
+
+ private class ProxyClientWebSocket : IClientWebSocket
+ {
+ private readonly ClientWebSocket client;
Review comment:
```suggestion
private readonly ClientWebSocket _client;
```
to follow .NET naming conventions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
> Gremlin.NET : WebSocketConnection does not check for MessageType.Close,
> causing error InvalidOperationException: "Received data deserialized into
> null object message. Cannot operate on it."
> ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: TINKERPOP-2717
> URL: https://issues.apache.org/jira/browse/TINKERPOP-2717
> Project: TinkerPop
> Issue Type: Bug
> Components: dotnet
> Affects Versions: 3.4.13, 3.5.2
> Reporter: Oliver Towers
> Priority: Minor
>
> h2. Issue:
> If the server sends a valid Close websocket message to the client, the client
> will throw {{InvalidOperationException}} rather than acknowledge/cleanup the
> connection gracefully.
> See relevant point where exception is thrown.
> [here|https://github.com/apache/tinkerpop/blob/7835e7a2c3b19a14f196525da844a2e35ba5a105/gremlin-dotnet/src/Gremlin.Net/Driver/Connection.cs#L123]
> Close messages from the server can be expected when the server is doing a
> graceful shutdown, and clients would be expected to reconnect.
> h2. Details:
> A Close websocket message being received by ClientWebSocket,
> [WebSocketReceiveResult.MessageType|https://docs.microsoft.com/en-us/dotnet/api/system.net.websockets.websocketreceiveresult.messagetype?view=net-6.0]
> will return {{MessageType.Close}}. In this case, the message buffer will be
> zero-bytes, see these
> [remarks|https://docs.microsoft.com/en-us/dotnet/api/system.net.websockets.websocketreceiveresult.count?view=net-6.0#remarks].
> Currently, Gremlin.NET does not check for this message type in
> [WebSocketConnection.ReceiveMessageAsync|https://github.com/apache/tinkerpop/blob/7835e7a2c3b19a14f196525da844a2e35ba5a105/gremlin-dotnet/src/Gremlin.Net/Driver/WebSocketConnection.cs#L92]
> and return a zero-length buffer. In
> [Connection.HandleReceiveAsync|https://github.com/apache/tinkerpop/blob/7835e7a2c3b19a14f196525da844a2e35ba5a105/gremlin-dotnet/src/Gremlin.Net/Driver/Connection.cs#L118],
> the attempt to deserialize the empty buffer returns `null` and the check
> fails.
> h2. Potential Fix
> # If `MessageType == Close` Throw a new connection closed exception from
> {{WebSocketConnection.ReceiveMessageAsync}} which includes
> {{WebSocketReceiveResult.CloseStatus}} and
> {{WebSocketReceiveResult.CloseDescription}} in the exception message and as
> properties.
> # This will be caught in {{Connection.HandleRecieveAsync}} and treated as
> fatal and will call {{CloseConnectionBecauseOfFailureAsync}} and will handle
> completing the close handshake, and notifies pending requests on the
> connection with the closed connection exception.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
