Sync with the plugin garbage collector when setting config #12152
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
This causes `PersistentPlugin` to wait for the plugin garbage collector to actually receive and process its config when setting it in `set_gc_config`. The motivation behind doing this is to make setting GC config in scripts more deterministic. Before this change we couldn't really guarantee that the GC could see your config before you started doing other things. There is a slight cost to performance to doing this - we set config before each plugin call because we don't necessarily know that it reflects what's in `$env.config`, and now to do that we have to synchronize with the GC thread. This was probably the cause of spuriously failing tests as mentioned by @sholderbach. Hopefully this fixes it. It might be the case that launching threads on some platforms (or just on a really busy test runner) sometimes takes a significant amount of time.
|
Oh, wow, it still happened here |
|
What is the verdict on the internal change? |
|
I believe this is still a good idea, because you'd expect config changes to take effect immediately in a script. The failing test is not the fault of this PR |
devyn
changed the title
|
You are the expert on this GC, so let's land it and see how it plays in the wild. |
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.
Description
This causes
PersistentPluginto wait for the plugin garbage collector to actually receive and process its config when setting it inset_gc_config.The motivation behind doing this is to make setting GC config in scripts more deterministic. Before this change we couldn't really guarantee that the GC could see your config before you started doing other things.
There is a slight cost to performance to doing this - we set config before each plugin call because we don't necessarily know that it reflects what's in
$env.config, and now to do that we have to synchronize with the GC thread.This was probably the cause of spuriously failing tests as mentioned by @sholderbach. Hopefully this fixes it. It might be the case that launching threads on some platforms (or just on a really busy test runner) sometimes takes a significant amount of time.
User-Facing Changes
Tests + Formatting
toolkit fmttoolkit clippytoolkit testtoolkit test stdlib