Fix completion refresher query results leak and active query error #161

MrMeemus · 2018-02-08T22:02:23Z

Fix for #103 and #95.

a.) A query is already in progress for this editor session. Please cancel this query or wait for its completion.

Sqltoolsservice was not designed for MARS (Multiple Active Result Set) support. Meaning it cannot handle concurrent queries for a single connection. The previous design for mssql-cli was to share a connection between the main thread and the completion refresher thread. Due to this, if a query from the main thread was executed at the same time as the query from the background, this error message would be populated.

Solution: Use a separate connection for the completion refresher thread. For Cloud support, this connection is new every time we refresh the completions when the database is altered or context changed.

b.) Query results from background completion refresher thread leaks onto main prompt.

When retrieving events from the shared JSON RPC client, a query/complete event would have no correlation with the request that submitted it. If the main thread executes in between the background threads query and fills the event queue with a query complete event, the background thread will break and never retrieve it's own results. The main thread then resumes and picks up the response from the background thread.

Solution: On top of maintaining a separate connection, responses are now retrieved via both a ID and/or Owner URI. This prevents connection A from retrieving results from connection B.

Added unit test:

Verifies query A does not retrieve responses from query B.

Additional fixes:

Logging for the completion refresher database connection.
Not failing the completion refresher when it cannot connect.
Correct casing for the error message event for the ResultSubset event.
Additional logging to expedite future issues.
Refactored and converged all JSON RPC contracts test into a single file as most of the code was redundant and hard to maintain.
Add'tl test changes mentioned in the commits.

… with addt'l logging.

…e code was redundant and hard to maintain.

…se queue when it's completed.

…urs.

…in addition to id.

… events based on it's owner uri.

pensivebrian · 2018-02-08T23:28:42Z

mssqlcli/main.py

        return [(None, None, None,
                 'Auto-completion refresh started in the background.')]

+    def get_completion_refresher_mssqlcliclient(self, mssqlcliclient):
+        mssqlcliclient_completion_refreshser = MssqlCliClient(


nit: naming - since we're creating a new MssqlCliClient, can we call this method create_completion_refresher_mssqlcliclient #Closed

pensivebrian · 2018-02-08T23:29:06Z

mssqlcli/main.py

        return [(None, None, None,
                 'Auto-completion refresh started in the background.')]

+    def get_completion_refresher_mssqlcliclient(self, mssqlcliclient):
+        mssqlcliclient_completion_refreshser = MssqlCliClient(


mssqlcliclient_completion_refreshser [](start = 8, length = 36)

spelling: refreshser #Closed

pensivebrian · 2018-02-08T23:29:53Z

mssqlcli/main.py

        return [(None, None, None,
                 'Auto-completion refresh started in the background.')]

+    def get_completion_refresher_mssqlcliclient(self, mssqlcliclient):
+        mssqlcliclient_completion_refreshser = MssqlCliClient(


MssqlCliClient [](start = 47, length = 14)

Perhaps we introduce a copy method to MssqlCliClient to centralize copying an instance? #Closed

I originally thought that would be a good idea, but then OOP wise I don't think a mssqlclient should be responsible for copying itself and generating a new owner URI. I moved that responsibility up a level. Let me know your thoughts. #Closed

Two options:

we currently generate an owner_id in the constructor, so can generate a new owner_id as part of the copy operation.

we can pass in the owner_id in the copy method.

I think I like #2, and if the value not passed, we create it:

if not owner_uri:
self.owner_uri = generate_owner_uri()

Maybe clone would be a better name to avoid shallow/deep copy confusion.

In reply to: 167103472 [](ancestors = 167103472)

pensivebrian · 2018-02-08T23:37:52Z

mssqlcli/jsonrpc/contracts/tests/test_json_rpc_contracts.py

+                                                    expected_complete_event=1)
+            rpc_client.shutdown()
+
+    def test_query_execute_response_AdventureWorks2014(self):


test_query_execute_response_AdventureWorks2014 [](start = 8, length = 46)

Can we add a test that does concurrent queries on two different owner uri?

Also, can we add any tests for non-happy path failure scenarios? For example, the refresh connection is killed while executing while the query connection continues. #Closed

That's what this test does. It ensures that Request A never retrieves responses for Request B. This was the root cause of the display leak which is why I targeted this type of unit test. I can add a query connection failed test.

Based on our discussion for restructuring the test story, I'll hold off on the failed connection test for now.

pensivebrian

🕐

pensivebrian · 2018-02-08T23:39:38Z

This PR is awesome! I can't wait to take this for a spin. Just some minor issues.

…esher.

pensivebrian · 2018-02-09T01:35:43Z

mssqlcli/main.py

        return [(None, None, None,
                 'Auto-completion refresh started in the background.')]

+    def create_completion_refresher_mssqlcliclient(self, mssqlcliclient):


create_completion_refresher_mssqlcliclient [](start = 8, length = 42)

I think we can delete this method can just call clone directly.

pensivebrian

MrMeemus · 2018-02-09T02:02:10Z

@pensivebrian ship it ribbit :)

…bcli#161) * Adding a lock acquisition during executing a single batch query. * Fixing casing in order to retrieve proper error message. * Adding support for owner uri in json rpc response dequeueing. * Adding support for a separate connection for the completion refrehser with addt'l logging. * Marking string as unicode for py2/3 compat. * Converging all sqltoolsservice test to a single file since most of the code was redundant and hard to maintain. * Adding missing json rpc client tests and reformatted for readability. * Adding clean up for each request to remove it's owner uri from response queue when it's completed. * Adding request finished for owner uri of request when a exception occurs. * Setting response_str to None to avoid assignment exception. * Fixing build.py format. * Adding owner URI to connection complete event during failure. * Changing get response signature in jsonrpcclient to accept owner uri in addition to id. * Removing copyright headers from files I touched. * Refactored test json rpc contracts tests. * Adding unit test for ensuring query execute requests retrieve correct events based on it's owner uri. * Fixing typo in refresher name. * Renaming method for creating a new mssqlcliclient for completion refresher. * Renamed baseline files for simplicity. * Removed copyright header. * Refactoring and adding clone support to mssqlcliclient. * Removing unnecessary call to get a new mssql cli client.

MrMeemus added 16 commits February 5, 2018 14:32

Adding a lock acquisition during executing a single batch query.

0a4f03d

Fixing casing in order to retrieve proper error message.

c5906a0

Adding support for owner uri in json rpc response dequeueing.

d13742c

Adding support for a separate connection for the completion refrehser…

3868cf1

… with addt'l logging.

Marking string as unicode for py2/3 compat.

b4e2eb9

Converging all sqltoolsservice test to a single file since most of th…

070eec3

…e code was redundant and hard to maintain.

Adding missing json rpc client tests and reformatted for readability.

7b045c9

Adding clean up for each request to remove it's owner uri from respon…

b1b11f4

…se queue when it's completed.

Adding request finished for owner uri of request when a exception occ…

793e1c1

…urs.

Setting response_str to None to avoid assignment exception.

47090c7

Fixing build.py format.

11c2983

Adding owner URI to connection complete event during failure.

2abca26

Changing get response signature in jsonrpcclient to accept owner uri …

41a2b97

…in addition to id.

Removing copyright headers from files I touched.

1d17a24

Refactored test json rpc contracts tests.

d6bc2ba

Adding unit test for ensuring query execute requests retrieve correct…

2549e00

… events based on it's owner uri.

MrMeemus requested review from pensivebrian and abhisheksinha89 February 8, 2018 22:02

MrMeemus self-assigned this Feb 8, 2018

MrMeemus added the bug label Feb 8, 2018

MrMeemus changed the title ~~Ron/query results completion displayed~~ Fix completion refresher query results leak and active query error Feb 8, 2018

pensivebrian reviewed Feb 8, 2018

View reviewed changes

pensivebrian requested changes Feb 8, 2018

View reviewed changes

MrMeemus added 3 commits February 8, 2018 15:54

Fixing typo in refresher name.

55cd00b

Renaming method for creating a new mssqlcliclient for completion refr…

c7e64d4

…esher.

Renamed baseline files for simplicity.

4f6774c

MrMeemus added 2 commits February 8, 2018 15:58

Removed copyright header.

bfb55f8

Refactoring and adding clone support to mssqlcliclient.

150e998

pensivebrian reviewed Feb 9, 2018

View reviewed changes

pensivebrian approved these changes Feb 9, 2018

View reviewed changes

Removing unnecessary call to get a new mssql cli client.

9a1c4a4

MrMeemus merged commit 959b652 into master Feb 9, 2018

MrMeemus deleted the ron/query_results_completion_displayed branch February 9, 2018 02:02

This was referenced Feb 9, 2018

Results from completion refresher can leak onto the prompt #103

Closed

A query is already in progress for this editor session #95

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix completion refresher query results leak and active query error #161

Fix completion refresher query results leak and active query error #161

MrMeemus commented Feb 8, 2018

pensivebrian Feb 8, 2018 •

edited

Loading

MrMeemus Feb 8, 2018

pensivebrian Feb 8, 2018 •

edited

Loading

MrMeemus Feb 8, 2018

pensivebrian Feb 8, 2018 •

edited

Loading

MrMeemus Feb 8, 2018 •

edited by pensivebrian

Loading

pensivebrian Feb 8, 2018

MrMeemus Feb 9, 2018

pensivebrian Feb 8, 2018 •

edited

Loading

MrMeemus Feb 8, 2018

MrMeemus Feb 9, 2018

pensivebrian left a comment

pensivebrian commented Feb 8, 2018

pensivebrian Feb 9, 2018

pensivebrian left a comment

MrMeemus commented Feb 9, 2018

Fix completion refresher query results leak and active query error #161

Fix completion refresher query results leak and active query error #161

Conversation

MrMeemus commented Feb 8, 2018

pensivebrian Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

MrMeemus Feb 8, 2018

Choose a reason for hiding this comment

pensivebrian Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

MrMeemus Feb 8, 2018

Choose a reason for hiding this comment

pensivebrian Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

MrMeemus Feb 8, 2018 • edited by pensivebrian Loading

Choose a reason for hiding this comment

pensivebrian Feb 8, 2018

Choose a reason for hiding this comment

MrMeemus Feb 9, 2018

Choose a reason for hiding this comment

pensivebrian Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

MrMeemus Feb 8, 2018

Choose a reason for hiding this comment

MrMeemus Feb 9, 2018

Choose a reason for hiding this comment

pensivebrian left a comment

Choose a reason for hiding this comment

pensivebrian commented Feb 8, 2018

pensivebrian Feb 9, 2018

Choose a reason for hiding this comment

pensivebrian left a comment

Choose a reason for hiding this comment

MrMeemus commented Feb 9, 2018

pensivebrian Feb 8, 2018 •

edited

Loading

pensivebrian Feb 8, 2018 •

edited

Loading

pensivebrian Feb 8, 2018 •

edited

Loading

MrMeemus Feb 8, 2018 •

edited by pensivebrian

Loading

pensivebrian Feb 8, 2018 •

edited

Loading