Multiple Connection Simple Execute #421
Conversation
| connInfo = null; | ||
| executeStringParams.OwnerUri = executeParams.OwnerUri; | ||
| } | ||
| await ConnectionService.Connect(connectParams); |
There was a problem hiding this comment.
Can this throw? Generally we're wrapping every JSON-RPC message handler with a try/catch where any error results in a requestContext.SendError being sent over the channel. I think we need it here as there's plenty going on that could throw.
| @@ -187,14 +190,21 @@ public void InitializeService(ServiceHost serviceHost) | |||
| ConnectionInfo connInfo; | |||
| if (!ConnectionService.TryFindConnection(executeParams.OwnerUri, out connInfo)) | |||
There was a problem hiding this comment.
Minor: I was confused by use of executeParams.OwnerUri vs executeStringParams.OwnerUri. One is the original request URI, the other is a random one used to ensure we have a dedicated connection. Given this, I'd suggest using a variable called randomUri or newUri instead of executeStringParams.OwnerUri.
| @@ -218,6 +232,10 @@ public void InitializeService(ServiceHost serviceHost) | |||
| { | |||
| await requestContext.SendError(SR.QueryServiceResultSetTooLarge); | |||
| ActiveQueries.TryRemove(executeStringParams.OwnerUri, out removedQuery); | |||
| ConnectionService.Disconnect(new DisconnectParams(){ | |||
There was a problem hiding this comment.
Why not make this a finally or something instead? You always want to disconnect, so let's ensure in the code that it happens no matter what the result of the query.
| queryService.ActiveQueries.TryGetValue(Constants.OwnerUri, out q); | ||
|
|
||
| // wait on the task to finish | ||
| Query q = queryService.ActiveQueries.Values.First(); |
There was a problem hiding this comment.
It would be nice to have a test that actually verifies we can run 2 queries against the same URI using this approach.
| Query removedQuery; | ||
| // remove the active query since we are done with it | ||
| ActiveQueries.TryRemove(randomUri, out removedQuery); | ||
| ConnectionService.Disconnect(new DisconnectParams(){ |
There was a problem hiding this comment.
I think you'll want to disconnect in all scenarios, in other words, I'd move this to the outside try/catch/finally block.
There was a problem hiding this comment.
Good question @kburtram would know more.
| ResultSetIndex = 0, | ||
| RowsStartIndex = 0, | ||
| RowsCount = Convert.ToInt32(rowCount) | ||
| OwnerUri = randomUri, |
There was a problem hiding this comment.
By connecting with a random URI, doesn't this incur a lot of overhead by kicking off intellisense discovery and whatnot?
Changes simple execute to open a new connection on every request and close it when it is finished. I'm not convinced this is a good solution, but it does work, hoping someone else will have a better idea.