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:
[email protected]
With regards,
Apache Git Services
