"flutter.setSubscriptions" call never forgets a file (aka will ask about an ever growing number of files)

[jensjoha]

In IntelliJ, opening a project with flutter in the environment in pubspec.yaml, e.g. something like

name: issue_whatnot
publish_to: "none"

environment:
  flutter: ">=3.29.0 <4.0.0"
  sdk: ">=3.7.0 <4.0.0"

opening a file sends a flutter.setSubscriptions request looking something like this:

{
  "id": "42",
  "method": "flutter.setSubscriptions",
  "params": {
    "subscriptions": {
      "OUTLINE": [
        "file:///path/to/file_1.dart"
      ]
    }
  }
}

Say we close the file and open another, it now sends something like this:

{
  "id": "42",
  "method": "flutter.setSubscriptions",
  "params": {
    "subscriptions": {
      "OUTLINE": [
        "file:///path/to/file_1.dart",
        "file:///path/to/file_2.dart"
      ]
    }
  }
}

closing that and opening a third, closing that and opening a forth etc a bunch of times and you'll end up with something like

{
  "id": "42",
  "method": "flutter.setSubscriptions",
  "params": {
    "subscriptions": {
      "OUTLINE": [
        "file:///path/to/file_1.dart",
        "file:///path/to/file_2.dart",
        ...
        "file:///path/to/file_n.dart",
      ]
    }
  }
}

at which point the analyzer will start to become slow:

5 ms (+ 22045 ms)	completion.getSuggestions2
23121 ms (+ 100 ms)	edit.getAssists

Notice that if I open a file I've already opened once it doesn't send the request, I believe because of this:

flutter-intellij/flutter-idea/src/io/flutter/dart/FlutterDartAnalysisServer.java

Lines 140 to 150 in 38e78f0

	private void addSubscription(@NotNull final String service, @NotNull final String filePath) {
	if(!isServerConnected()) {
	return;
	}
	final List<String> files = subscriptions.computeIfAbsent(service, k -> new ArrayList<>());
	final String filePathOrUri = getAnalysisService().getLocalFileUri(filePath);
	if (!files.contains(filePathOrUri)) {
	files.add(filePathOrUri);
	sendSubscriptions();
	}
	}

.

(i.e. sendSubscriptions is only called i it actually adds the path)

I'm guessing something goes wrong in these lines:

flutter-intellij/flutter-idea/src/io/flutter/editor/ActiveEditorsOutlineService.java

Lines 125 to 152 in 38e78f0

	final List<String> obsoletePaths = new ArrayList<>();
	synchronized (outlineListeners) {
	for (final String path : outlineListeners.keySet()) {
	if (!newPaths.contains(path)) {
	obsoletePaths.add(path);
	}
	}
	for (final String path : obsoletePaths) {
	final String filePathOrUri = getAnalysisServer().getAnalysisService().getLocalFileUri(path);
	final FlutterOutlineListener listener = outlineListeners.remove(filePathOrUri);
	if (listener != null) {
	getAnalysisServer().removeOutlineListener(FileUtil.toSystemDependentName(path), listener);
	}
	}
	// Register new outline listeners.
	for (final String path : newPaths) {
	final String filePathOrUri = getAnalysisServer().getAnalysisService().getLocalFileUri(path);
	if (outlineListeners.containsKey(filePathOrUri)) {
	continue;
	}
	final FlutterOutlineListener listener = new OutlineListener(filePathOrUri);
	outlineListeners.put(filePathOrUri, listener);
	getAnalysisServer().addOutlineListener(FileUtil.toSystemDependentName(path), listener);
	}
	}

Without having looked into it it seems suspect that it takes a key from outlineListeners (final String path : outlineListeners.keySet()) and then does something to it before trying to remove it again (final String filePathOrUri = getAnalysisServer().getAnalysisService().getLocalFileUri(path);, final FlutterOutlineListener listener = outlineListeners.remove(filePathOrUri);), but I'm guessing.

[jensjoha]

/cc @jwren

[bwilkerson]

@jwren Is it true that IntelliJ should never need to get flutter outline information for more than one file at a time?

[jwren]

I am not familiar enough with the Flutter setSubscriptions protocol (https://htmlpreview.github.io/?https://github.com/dart-lang/sdk/blob/main/pkg/analysis_server/doc/api.html#request_flutter.setSubscriptions) to be able to come in and determine if the right things are being done here from the IJ side without lots of manual testing and digging into the protocol. It looks like the feature was largely added by Konstantin and Jacob. Thanks for starting to look into this issue to get better performance.

[pq]

Thanks for this great report @jensjoha!

Taking a look at this now.

[alexander-doroshko]

The the fileOutlineListeners map grows indefinitely because

• addOutlineListener uses getLocalFileUri(filePath) as a map key
• removeOutlineListener assumes that a plain filePath is used as a key

If all code related to the "flutter.setSubscriptions" call is not needed anymore, this map may be completely removed along with a bunch of other obsolete code

[pq]

If all code related to the "flutter.setSubscriptions" call is not needed anymore, this map may be completely removed along with a bunch of other obsolete code

Right! I'm trying to establish exactly this. It looks like CommonTestConfigUtils uses this service (see, for example, getTestsFromOutline) but I'm not sure if this is being used (or works as intended). Will dig a little more.

Thanks!

[pq]

On the topic of managing the file map, it seems like the issue is introduced (or at least manifest) here:

flutter-intellij/flutter-idea/src/io/flutter/editor/ActiveEditorsOutlineService.java

Lines 134 to 136 in 38e78f0

	for (final String path : obsoletePaths) {
	final String filePathOrUri = getAnalysisServer().getAnalysisService().getLocalFileUri(path);
	final FlutterOutlineListener listener = outlineListeners.remove(filePathOrUri);

The rub is that the path in obsoletePaths is already a file path (for example file:///Users/pq/StudioProjects/fluttery/lib/main.dart). When such a path is passed to getLocalFileUri it gets turned into something like file://file:///Users/pq/StudioProjects/fluttery/lib/main.dart, which is incorrect and so the listener is never removed.

@alexander-doroshko: it seems like getLocalFileUri could be improved in the Dart plugin DartAnalysisServerService to do the right thing in case a valid file path is passed into it?

In the meantime, I think using the obsolete path as-is addresses the problem:

for (final String path : obsoletePaths) {
  final FlutterOutlineListener listener = outlineListeners.remove(path);
  ...

[pq]

As for the use of the flutter outline service in general, even though we are no longer displaying a flutter outline view, it does seem like we're using the the service to figure out where to contribute test line markers.

See:

flutter-intellij/flutter-idea/src/io/flutter/run/common/TestLineMarkerContributor.java

Line 63 in a801049

final TestType testCall = testConfigUtils.asTestCall(dartCallExpression);

which calls here:

flutter-intellij/flutter-idea/src/io/flutter/run/common/CommonTestConfigUtils.java

Line 59 in 9a6aff3

public TestType asTestCall(@NotNull PsiElement element) {

I've confirmed that this code is alive and does contribute to markers showing up

but I'm not sure it's entirely working as intended. Either way, in the absences of a flutter outline view, it's likely this isn't the best way to calculate test line markers regardless but I'm missing a lot of context and need to do some more digging but maybe we should create another issue for that and just focus on getting the subscribed file list to stop growing infinitely.

[alexander-doroshko]

The rub is that the path in obsoletePaths is already a file path (for example file:///Users/pq/StudioProjects/fluttery/lib/main.dart)

You mean 'already a URI' :)

@alexander-doroshko: it seems like getLocalFileUri could be improved in the Dart plugin DartAnalysisServerService to do the right thing in case a valid file path is passed into it?

You mean 'to do the right thing in case a valid file URI is passed into it' :)

I'd say passing URI to DartAnalysisServerService.getLocalFileUri() is an incorrect API usage. I think getLocalFileUri() shouldn't try to fix caller mistake. I'd rather make it throw IllegalArgumentException. If it had done that, we'd have caught the problem a year ago.

[pq]

You mean 'already a URI' :)

Yes, right! The trouble is, when we're passing strings around, it's hard to tell. 😄

That said, if getLocalFileUri threw an exception we'd be better off for sure.

Anyway, I'll put together a fix for this and if things get hardened up-stream, all the better.

Thanks!

[alexander-doroshko]

we're passing strings around, it's hard to tell. 😄

True.
Macros are gone but code complexity remains :)
Just FYI, here’s an approach I followed while migrating from paths to URIs in the Dart plugin. This could help with reading the Dart plugin source code and updating the Flutter plugin.

Note that this text was written a year ago. Given that the Flutter plugin has been already updated (hopefully, according to these recommendations), and that macros have gone since then, this text might be partially obsolete.

1. Sending data to the Analysis Server

• try not to work with paths, work with VirtualFiles
• in some cases working with VirtualFile is impossible, you need to work with paths, but you must be sure that those are local file paths. Because such thing as ‘macro-generated’ file path doesn’t make sense, those files have only URIs
• convert VirtualFile to path or uri as late as possible, right before sending to the Analysis Server, don’t use this string (path or uri) for anything else
• com.jetbrains.lang.dart.analyzer.DartAnalysisServerService#getFileUri does the trick
  • for SDK versions 3.3 and older it returns plain file path with system-dependent slashes
  • for SDK versions 3.4 and newer it returns URI, as expected by the server
• when you don’t have a VirtualFile but only have a file path and need to send it to the AnalysisServer
  • you must be sure that this is a local file path. Such thing as ‘macro-generated’ file path doesn’t make sense
  • use com.jetbrains.lang.dart.analyzer.DartAnalysisServerService#getLocalFileUri
  • for SDK versions 3.3 and older it returns plain file path with system-dependent slashes
  • for SDK versions 3.4 and newer it returns URI, as expected by the server
  • again, don’t use the returned string for anything else, only for sending to the server
• example: DartAnalysisServerService.search_findElementReferences and more in this class
• The methods getFileUri and getLocalFileUri are public for the Flutter plugin. In the Dart plugin they are used only in the DartAnalysisServerService class and are not expected to be used anywhere else

2. Handling data received from the AnalysisServer

• use filePathOrUri name for parameters/variables that store strings received from the Analysis Server
• after receiving, immediately call fileInfo=com.jetbrains.lang.dart.analyzer.DartFileInfoKt#getDartFileInfo(project, filePathOrUri)
• don’t use the received filePathOrUri string anymore, use DartFileInfo object
• most likely you’ll only need com.jetbrains.lang.dart.analyzer.DartFileInfo#findFile
• in rare cases you may need to check if it is an instance of DartLocalFileInfo or DartNotLocalFileInfo and use DartLocalFileInfo.filePath or DartNotLocalFileInfo.fileUri. Think twice whether you really need it

3. Looking for code in the plugin that needs to be updated

• all getFile() methods of all classes in org.dartlang.analysis.server.protocol now return filePathOrUri. For example, Position.getFile(). I haven’t found such usages in the Flutter plugin but it would be good to double-check
• all calls of all methods of the com.google.dart.server.internal.remote.RemoteAnalysisServerImpl class
• all implementations of Analysis Server-related listeners. Like com.google.dart.server.AnalysisServerListener. Its methods now get file path or uri. For example, com.google.dart.server.AnalysisServerListener#computedErrors(String file, List<AnalysisError> errors). Its parameter is called file, but it’s not always a file path. Call the parameter filePathOrUri in the implementation
• check all toSystemDependentName() and toSystemIndependentName(). The rule of thumb is:
  • call toSystemDependentName() as late as possible to show the text in UI. for example, textField.setText(toSystemDependentName(file.getPath))
  • call toSystemIndependentName() as early as possible right after getting the text from a UI component
  • if you need to work with paths, work only with system independent paths everywhere else
• check all LocalFileSystem.findXxx() methods. Use DartFileInfo.findFile() instead where needed.