Share logging file with Dart plugin

[helin24]

See flutter/dart-intellij-third-party#276 and #8834

[gemini-code-assist]

Code Review

This pull request modifies the logging initialization to share a single log file (dash.log) between the Flutter and Dart IntelliJ plugins. This is achieved by checking if a FileHandler has already been set up by the other plugin and reusing it. If not, it creates a new handler and attaches it to both plugins' loggers. While this approach addresses the file contention issue, the current implementation has a potential race condition during initialization and could be simplified to improve readability and maintainability. My review includes suggestions to make the initialization thread-safe and to refactor duplicated code.

[helin24]

/gemini review

[gemini-code-assist]

Code Review

This pull request aims to share a logging file between the Flutter and Dart IntelliJ plugins. While the intent is good, the core implementation in PluginLogger.java has some significant logical flaws that prevent it from working as intended. Specifically, the logic for finding and reusing an existing log handler is incorrect, and the coordination with the Dart plugin's logger is missing. I've provided a critical comment detailing the issues and suggesting a simpler, more robust approach. Additionally, there are a couple of medium-severity issues related to code maintainability and adherence to platform best practices that should be addressed.

[helin24]

/gemini review

[gemini-code-assist]

Code Review

The pull request refactors the PluginLogger to share a single log file handler between the Flutter and Dart IntelliJ plugins. The logic correctly handles cases where either plugin might initialize first by checking for an existing handler before creating a new one. My review identifies a few key areas for improvement. First, a potential UI freeze issue due to performing file I/O in a static class initializer, which violates the project's threading guidelines. Second, the use of System.setProperty to configure the log format, which can cause side effects across the IDE; I've suggested a safer alternative. Finally, I've provided a suggestion to simplify one of the new helper methods for better readability.

[helin24]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the plugin logging to allow sharing a log file with the Dart plugin, which is a great step for unified diagnostics. The approach to find and reuse an existing FileHandler is sound. However, I've identified a critical bug in the new initialization logic where an error during setup could leave logging in a broken state. I've also noted a couple of style guide violations regarding logger usage and method length. My suggested change addresses the critical bug and one of the style issues to improve the robustness of the new logic.

[gemini-code-assist]

There are a couple of issues in this block:

• [MUST-FIX] Bug: The isInitialized flag is set to true at the start of the synchronized block. If an exception occurs during handler creation (lines 60-70), subsequent calls to initLogger() will incorrectly assume initialization was successful and do nothing, leaving logging broken. The flag should only be set after all initialization has succeeded.
• [CONCERN] Style: The repository style guide (rule Infra - project model #28) requires using the IntelliJ platform logger (com.intellij.openapi.diagnostic.Logger). The current code uses java.util.logging.Logger directly in the catch block.
• [CONCERN] Maintainability: This method exceeds the 30-line limit recommended by the style guide (rule Allow for the creation of multiple Flutter SDKs #55), which can make it harder to maintain.

The suggested change below fixes the critical bug and the logging style violation. You may also consider refactoring this logic into smaller helper methods for improved readability.

    synchronized (LogManager.getLogManager()) {
      if (isInitialized) return; // Double-checked locking

      // Try to find an existing FileHandler on either logger
      FileHandler existingHandler = getExistingFileHandler(rootLogger);
      if (existingHandler == null) {
        existingHandler = getExistingFileHandler(dartLogger);
      }

      if (existingHandler != null) {
        // Another plugin initialized first; reuse its handler
        ensureHandlerSet(rootLogger, existingHandler);
      }
      else {
        // We are the first plugin to initialize; create the handler
        try {
          FileHandler newHandler = new FileHandler(fullPath, MAX_LOG_SIZE, MAX_LOG_FILES, true);
          newHandler.setFormatter(createLogFormatter());

          // Attach to logger so the next plugin finds it
          ensureHandlerSet(rootLogger, newHandler);
        }
        catch (IOException | SecurityException e) {
          // Per style guide, use the platform logger.
          com.intellij.openapi.diagnostic.Logger.getInstance(PluginLogger.class)
            .warn("Failed to initialize plugin log file handler", e);
          // Do not mark as initialized if handler creation failed.
          return;
        }
      }

      isInitialized = true;
    }

References

1. Enforce the use of the IntelliJ SDK's built-in logger: com.intellij.openapi.diagnostic.Logger or our own: io.flutter.logging.PluginLogger. ^(link)
2. Methods should ideally be 10-20 lines. If a method exceeds 30 lines, suggest a refactor. ^(link)

[helin24]

@gemini-code-assist if logger setup fails, I'm not sure we would want to retry anytime there is a plugin logger log. It may be more reasonable to let the setup fail and miss logs (and debug why it failed separately), rather than repeat a broken process.

the use of default logger is necessary because we want to see a message that plugin logger failed in idea.log.

[gemini-code-assist]

I understand your concern, @helin24. You're right that retrying the logger setup repeatedly might not be the best approach. It's a trade-off between potentially fixing a transient issue and avoiding a flood of error messages. Given the context, prioritizing the visibility of the initial setup failure in idea.log seems reasonable.

Perhaps a good compromise would be to add a mechanism to retry the logger setup only a limited number of times, or after a certain delay, to avoid overwhelming the logs. However, I agree with your assessment that for now, the current approach is acceptable.