Object Explorer session connection mapping accounts for default connection values#520
Merged
mmcfarland merged 1 commit intomainfrom Mar 3, 2025
Merged
Conversation
A recent change to the client behavior meant that the object explorer would send a standard ownerUri based on connection details. Previously, the inconsistency meant that the map of connections tracked by the connection manager actually had two different connections stored. Now that a single URI was used, the tools service should have been able to just use the single connection. However, the implementation caused a bug and consistent race condition. When the connection manager got the request the for second connection request, it would now correctly see that it already had a connection for these details (via the URI). It then checked to see if the connection details had changed. Because the incoming details didn't have the default values (like port, dbname) applied to it, but did have them on the details stored from the connection that was actually opened, it incorrectly believed it to be a "new" connection for the same server. It then closed the connection to re-create it later. Meanwhile, the client has likely requested nother operation involving the connection, which was possibly closed, resulting in a failure. By extending the equality check to account for default values, the tools service should no longer close connections when the details haven't actually changed.
p-singh-ms
approved these changes
Mar 3, 2025
Contributor
|
A much more robust match check :) |
lossyrob
approved these changes
Mar 3, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A recent change to the client behavior meant that the object explorer would send a standardized
ownerUribased on connection details. Previously, the inconsistency meant that the map of connections tracked by the tools service connection manager actually had two different connections stored for the same object explorer session.Now that a single URI was used, the tools service should have been able to just use the single connection. However, the existing implementation caused a bug and consistent race condition. When the connection manager got the request the for second connection request, it would now correctly see that it already had a connection for these details (via the URI). It then checked to see if the connection details had changed. In the cases where client-optional values weren't set (like port, dbname), the details comparison would fails since those values were added to the details when the actual server connection was opened. The service incorrectly believed that connection details had changed, so it closed the connection to re-create it later.
Meanwhile, the client has likely requested another operation involving the connection, which was possibly closed from above, resulting in a failure. By extending the equality check to account for default values, the tools service should no longer close connections when the details haven't actually changed.