Pass in project when querying flutter channel#8883
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the FlutterSdk.queryFlutterChannel method to accept a Project parameter, ensuring the correct Git executable is used, and expands exception handling to include IllegalStateException. Review feedback highlights a thread-safety concern regarding the use of HashMap for cachedConfigValues when accessed from background threads, and identifies a potential lock leak in FlutterSettingsConfigurable that should be addressed by releasing the lock within a finally block.
|
@gemini-code-assist review again |
There was a problem hiding this comment.
Code Review
This pull request improves thread safety by switching the SDK configuration cache to a ConcurrentHashMap and enhances the queryFlutterChannel method to utilize the project context when resolving the git executable. Additionally, it ensures robust lock management in the settings configuration. Review feedback highlights a potential NullPointerException risk since ConcurrentHashMap does not permit null values, and recommends using Optional for the new method's return type to comply with the project's style guide.
| private final @NotNull VirtualFile myHome; | ||
| private final @NotNull FlutterSdkVersion myVersion; | ||
| private final @NotNull Map<String, String> cachedConfigValues = new HashMap<>(); | ||
| private final @NotNull Map<String, String> cachedConfigValues = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
[MUST-FIX]: The switch to ConcurrentHashMap is a good improvement for thread safety. However, unlike HashMap, ConcurrentHashMap does not allow null values. In queryFlutterChannel (line 616), the branch variable (assigned from getLocation or parseChannel) could potentially be null, which would cause a NullPointerException when calling cachedConfigValues.put("channel", branch). Please ensure that a non-null default value (e.g., "unknown") is used if the result is null.
|
|
||
| @Nullable | ||
| @NonNls | ||
| public FlutterSdkChannel queryFlutterChannel(boolean useCachedValue, @Nullable Project project) { |
There was a problem hiding this comment.
[NIT]: According to the Repository Style Guide (Rule 65), java.util.Optional should be used for return types that may be empty instead of returning null. Since this is a new overloaded method, consider returning Optional<FlutterSdkChannel> to align with modern Java practices in this codebase.
References
- Use java.util.Optional for return types that may be empty; avoid returning null. (link)
|
I believe this is no longer needed since the project open issue was fixed. |
1 participant
Partially fixes #8882
I was only able to reproduce the error on IDEA 2026.1, not in older versions. I verified that starting a new project with this fix in place did not show the error.