[GitHub] [tinkerpop] spmallette commented on a change in pull request #1263: dotnet: add session mode connection

[GitBox <gi...@apache.org>]

spmallette commented on a change in pull request #1263: dotnet: add session mode connection
URL: https://github.com/apache/tinkerpop/pull/1263#discussion_r394600294
 
 

 ##########
 File path: gremlin-dotnet/src/Gremlin.Net/Driver/Connection.cs
 ##########
 @@ -235,17 +243,52 @@ private void NotifyAboutConnectionFailure(Exception exception)
 
         private async Task SendMessageAsync(RequestMessage message)
         {
+            if (_sessionEnabled)
+            {
+                message = RebuildSessionMessage(message);
+            }
             var graphsonMsg = _graphSONWriter.WriteObject(message);
             var serializedMsg = _messageSerializer.SerializeMessage(graphsonMsg);
             await _webSocketConnection.SendMessageAsync(serializedMsg).ConfigureAwait(false);
         }
 
+        private RequestMessage RebuildSessionMessage(RequestMessage message)
+        {
+            if (message.Processor == Tokens.OpsAuthentication)
+            {
+                return message;
+            }
+
+            var msgBuilder = RequestMessage.Build(message.Operation)
+              .OverrideRequestId(message.RequestId).Processor(Tokens.ProcessorSession);
+            foreach(var kv in message.Arguments)
+            {
+                msgBuilder.AddArgument(kv.Key, kv.Value);
+            }
+            msgBuilder.AddArgument(Tokens.ArgsSession, _sessionId);
+            return msgBuilder.Create();
+        }
+
         public async Task CloseAsync()
         {
             Interlocked.Exchange(ref _connectionState, Closed);
+
+            if (_sessionEnabled)
+            {
+                await CloseSession().ConfigureAwait(false);
+            }
             await _webSocketConnection.CloseAsync().ConfigureAwait(false);
         }
 
+        private async Task CloseSession()
+        {
+            // build a request to close this session, 'gremlin' in args as tips and not be executed actually
+            var msg = RequestMessage.Build(Tokens.OpsClose).Processor(Tokens.ProcessorSession)
+              .AddArgument(Tokens.ArgsGremlin, "session.close()").Create();
 
 Review comment:
   It seems we accepted this pattern here for Javascript:
   
   https://github.com/apache/tinkerpop/commit/1751d25ad4b664a70f254ac2822bdae729014256#diff-9ec1348d608a722cece8543e6db87a23R225
   
   I think I overlooked that in review. I don't think we should include an arg just for the sake of it. I would also go a step further and say that we probably shouldn't bother to add support for the "close" message at all.  We just deprecated the "close" message in a recent batch of changes and no longer send it at all in the Java driver in 3.5.0:
   
   https://github.com/apache/tinkerpop/blob/0a7769eea651ab0e72d3a6a0bd424fbb1197bd47/docs/src/upgrade/release-3.5.x.asciidoc#session-close
   
   I don't think it's worth adding now, unless we can come up with a good reason for doing so. The only argument I can think of is that it would make the drivers backward compatible with sessions starting at 3.4.7, but even then the "close" message is a bit flawed on the server in those earlier versions:
   
   https://issues.apache.org/jira/browse/TINKERPOP-2336
   
   so, is there real value to adding the code on the client? If the answer to that is "yes" with some reasoning then we'd need a second PR to remove the code on the "master" branch to match the Java Driver. Thoughts?
   
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services