(GH-180) Ensure instance_key respects full uniqueness of options #181
Conversation
This file contains 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
Prior to this commit, the instance_key method for the PowerShell Manager only partially respected the uniqueness of a possible instance; if there are any differences between two declarations in the path to the executable, arguments passed to the executable, or the debug option, the manager would treat the new instance as expected, spinning up an additional instance of the manager. However, it completely ignored the pipe_timeout option, meaning specifying an instance with a default pipe_timeout and a second instance with a pipe_timeout of 45 would **actually** result in the second instance declaration just reusing the existing instance with the default timeout, which is unwanted an unexpected behavior. This commit modifies the logic for the instance_key method slightly by making the key a concatenation of the path to the executable, the arguments, and the full options hash turned into a string. This both ensures that instance declarations with two different pipe timeouts are actually both spun up as separate instances *and* enables a workaround for multi-threading: Users will be able to specify an arbitrary option, such as instance_guid, which can uniquely identify the instance for that thread. This should help folks keep the instances separated and prevent weird behavior when calling the manager from multiple threads (though, of course, this does mean that the multi-threading still has a spinup cost for each thread). Related to/discovered in #180.
da-ar
approved these changes
Aug 16, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Prior to this commit, the instance_key method for the PowerShell Manager only partially
respected the uniqueness of a possible instance; if there are any differences between
two declarations in the path to the executable, arguments passed to the executable, or
the debug option, the manager would treat the new instance as expected, spinning up an
additional instance of the manager.
However, it completely ignored the pipe_timeout option, meaning specifying an instance
with a default pipe_timeout and a second instance with a pipe_timeout of 45 would
actually result in the second instance declaration just reusing the existing instance
with the default timeout, which is unwanted an unexpected behavior.
This commit modifies the logic for the instance_key method slightly by making the key a
concatenation of the path to the executable, the arguments, and the full options hash
turned into a string.
This both ensures that instance declarations with two different pipe timeouts are actually
both spun up as separate instances and enables a workaround for multi-threading:
Users will be able to specify an arbitrary option, such as instance_guid, which can uniquely
identify the instance for that thread. This should help folks keep the instances separated
and prevent weird behavior when calling the manager from multiple threads (though, of course,
this does mean that the multi-threading still has a spinup cost for each thread).
Related to/discovered in #180.