Skip to content

Commit

Permalink
Revert "analyzer: Do not overwrite an original exception when a plugi…
Browse files Browse the repository at this point in the history 
…n crashes"

This reverts commit 7784cf3.

Reason for revert: broke windows bot

Original change's description:
> analyzer: Do not overwrite an original exception when a plugin crashes
>
> Maybe related to #38629. These tests have been skipped for so long, enabling them took some work, to migrate them from '.packages' files to package config files.
>
> Some other tidying in the test file:
>
> * inline `byteStorePath`, only used once.
> * simplify `_packagesFileContent` and `_getPackagesFileContent`
>   into a static getter.
> * simplify `_defaultPluginContent` into a const String, so it can
>   be used as a function parameter default value
>
> The diff is way bigger than the functional changes, because we sort
> elements.
>
> Change-Id: I193316316750e80268b684fdc1abe558a77994fe
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/344601
> Reviewed-by: Brian Wilkerson <[email protected]>
> Commit-Queue: Samuel Rawlins <[email protected]>

Change-Id: Ibeb761afebad4fb4166cec756743dbb35d323e7d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/345143
Bot-Commit: Rubber Stamper <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
  • Loading branch information
@srawlins
srawlins authored and Commit Queue committed Jan 5, 2024
1 parent e6d1333 commit aa6b647
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 136 deletions.
6 changes: 2 additions & 4 deletions pkg/analysis_server/lib/src/plugin/plugin_manager.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,9 +196,7 @@ abstract class PluginInfo {
}

void reportException(CaughtException exception) {
// If a previous exception has been reported, do not replace it here; the
//first should have more "root cause" information.
_exception ??= exception;
_exception = exception;
instrumentationService.logPluginException(
data, exception.exception, exception.stackTrace);
}
Expand Down Expand Up @@ -974,7 +972,7 @@ class PluginSession {
onDone: handleOnDone, onError: handleOnError) as dynamic);
if (channel == null) {
// If there is an error when starting the isolate, the channel will invoke
// `handleOnDone`, which will cause `channel` to be set to `null`.
// handleOnDone, which will cause `channel` to be set to `null`.
info.reportException(CaughtException(
PluginException('Unrecorded error while starting the plugin.'),
StackTrace.current));
Expand Down
238 changes: 106 additions & 132 deletions pkg/analysis_server/test/src/plugin/plugin_manager_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,12 +187,13 @@ class DiscoveredPluginInfoTest with ResourceProviderMixin, _ContextRoot {

@reflectiveTest
class PluginManagerFromDiskTest extends PluginTestSupport {
String byteStorePath = '/byteStore';
late PluginManager manager;

@override
void setUp() {
super.setUp();
manager = PluginManager(resourceProvider, '/byteStore', '',
manager = PluginManager(resourceProvider, byteStorePath, '',
notificationManager, InstrumentationService.NULL_SERVICE);
}

Expand Down Expand Up @@ -283,6 +284,9 @@ class PluginManagerFromDiskTest extends PluginTestSupport {
pkg1Dir.deleteSync(recursive: true);
}

@SkippedTest(
reason: 'flaky timeouts',
issue: 'https://github.com/dart-lang/sdk/issues/38629')
Future<void> test_broadcastRequest_noCurrentSession() async {
var pkg1Dir = io.Directory.systemTemp.createTempSync('pkg1');
var pkgPath = pkg1Dir.resolveSymbolicLinksSync();
Expand All @@ -294,26 +298,11 @@ class PluginManagerFromDiskTest extends PluginTestSupport {
await manager.addPluginToContextRoot(contextRoot, plugin1Path);

var responses = manager.broadcastRequest(
CompletionGetSuggestionsParams('/pkg1/lib/pkg1.dart', 100),
contextRoot: contextRoot,
);
CompletionGetSuggestionsParams('/pkg1/lib/pkg1.dart', 100),
contextRoot: contextRoot);
expect(responses, hasLength(0));

await manager.stopAll();
var exception = manager.plugins.first.exception;
expect(exception, isNotNull);
var innerException = exception!.exception;
expect(
innerException,
isA<PluginException>().having(
(e) => e.message,
'message',
allOf(
contains('Unable to spawn isolate'),
contains('(invalid content here)'),
),
),
);
});
pkg1Dir.deleteSync(recursive: true);
}
Expand Down Expand Up @@ -452,12 +441,7 @@ class PluginManagerFromDiskTest extends PluginTestSupport {
}

ContextRootImpl _newContextRoot(String root) {
root = resourceProvider.convertPath(root);
return ContextRootImpl(
resourceProvider,
resourceProvider.getFolder(root),
BasicWorkspace.find(resourceProvider, Packages.empty, root),
);
throw UnimplementedError();
}
}

Expand Down Expand Up @@ -705,91 +689,28 @@ class PluginSessionTest with ResourceProviderMixin {
}
}

/// A superclass for test classes that define tests that require plugins to be
/// created on disk.
/// A class designed to be used as a superclass for test classes that define
/// tests that require plugins to be created on disk.
abstract class PluginTestSupport {
/// The default content of the plugin. This is a minimal plugin that will only
/// respond correctly to version checks and to shutdown requests.
static const _defaultPluginContent = r'''
import 'dart:async';
import 'dart:isolate';
import 'package:analyzer/file_system/file_system.dart';
import 'package:analyzer/file_system/physical_file_system.dart';
import 'package:analyzer/src/dart/analysis/driver.dart';
import 'package:analyzer_plugin/plugin/plugin.dart';
import 'package:analyzer_plugin/protocol/protocol_generated.dart';
import 'package:analyzer_plugin/starter.dart';
import 'package:pub_semver/pub_semver.dart';
void main(List<String> args, SendPort sendPort) {
MinimalPlugin plugin = new MinimalPlugin(PhysicalResourceProvider.INSTANCE);
new ServerPluginStarter(plugin).start(sendPort);
}
class MinimalPlugin extends ServerPlugin {
MinimalPlugin(ResourceProvider provider) : super(provider);
@override
List<String> get fileGlobsToAnalyze => <String>['**/*.dart'];
@override
String get name => 'minimal';
@override
String get version => '0.0.1';
@override
AnalysisDriverGeneric createAnalysisDriver(ContextRoot contextRoot) => null;
@override
Future<AnalysisHandleWatchEventsResult> handleAnalysisHandleWatchEvents(
AnalysisHandleWatchEventsParams parameters) async =>
new AnalysisHandleWatchEventsResult();
@override
bool isCompatibleWith(Version serverVersion) => true;
}
''';

/// The content to be used for the package config file.
static String get _packageConfigContent {
var sdkPackagesFile = io.File(_sdkPackageConfigPath);
var sdkPackageMap = sdkPackagesFile.readAsLinesSync();
return _convertPackageMap(
path.dirname(sdkPackagesFile.path), sdkPackageMap);
}

/// The path to the package config file in the root of the Dart SDK.
static String get _sdkPackageConfigPath {
var filePath = io.Platform.script.toFilePath();
// Walk up the directory structure from this script file to the
// 'analysis_server' root directory.
while (
filePath.isNotEmpty && path.basename(filePath) != 'analysis_server') {
filePath = path.dirname(filePath);
}
filePath = path.dirname(path.dirname(filePath));
return path.join(filePath, '.dart_tool', 'package_config.json');
}

late PhysicalResourceProvider resourceProvider;

late TestNotificationManager notificationManager;

/// The content to be used for the '.packages' file, or `null` if the content
/// has not yet been computed.
String? _packagesFileContent;

void setUp() {
resourceProvider = PhysicalResourceProvider.INSTANCE;
notificationManager = TestNotificationManager();
}

/// Creates a directory structure representing a plugin on disk, runs the
/// given [test] function, and then removes the directory.
///
/// The directory will have the following structure:
/// Create a directory structure representing a plugin on disk, run the given
/// [test] function, and then remove the directory. The directory will have
/// the following structure:
/// ```
/// <pluginDirectory>
/// .dart_tool/
/// package_config.dart
/// bin/
/// pluginDirectory
/// .packages
/// bin
/// plugin.dart
/// ```
/// The name of the plugin directory will be the [pluginName], if one is
Expand All @@ -798,21 +719,19 @@ class MinimalPlugin extends ServerPlugin {
/// content that implements a minimal plugin if the contents are not given.
/// The [test] function will be passed the path of the directory that was
/// created.
Future<void> withPlugin({
String content = _defaultPluginContent,
String pluginName = 'test_plugin',
required Future<void> Function(String) test,
}) async {
var tempDirectory = io.Directory.systemTemp.createTempSync(pluginName);
Future<void> withPlugin(
{String? content,
String? pluginName,
required Future<void> Function(String) test}) async {
var tempDirectory =
io.Directory.systemTemp.createTempSync(pluginName ?? 'test_plugin');
try {
var pluginPath = tempDirectory.resolveSymbolicLinksSync();
// Create a package config file.
var pluginDartToolPath = path.join(pluginPath, '.dart_tool');
io.Directory(pluginDartToolPath).createSync();
var packageConfigFile = io.File(
path.join(pluginDartToolPath, 'package_config.json'),
);
packageConfigFile.writeAsStringSync(_packageConfigContent);
//
// Create a .packages file.
//
var packagesFile = io.File(path.join(pluginPath, '.packages'));
packagesFile.writeAsStringSync(_getPackagesFileContent());
//
// Create the 'bin' directory.
//
Expand All @@ -822,7 +741,7 @@ class MinimalPlugin extends ServerPlugin {
// Create the 'plugin.dart' file.
//
var pluginFile = io.File(path.join(binPath, 'plugin.dart'));
pluginFile.writeAsStringSync(content);
pluginFile.writeAsStringSync(content ?? _defaultPluginContent());
//
// Run the actual test code.
//
Expand All @@ -832,25 +751,94 @@ class MinimalPlugin extends ServerPlugin {
}
}

/// Converts the [sdkPackageMap] into a plugin-specific map.
static String _convertPackageMap(
String sdkDirPath, List<String> sdkPackageMap) {
/// Convert the [sdkPackageMap] into a plugin-specific map by applying the
/// given relative path [delta] to each line.
String _convertPackageMap(String sdkDirPath, List<String> sdkPackageMap) {
var buffer = StringBuffer();
for (var line in sdkPackageMap) {
if (!line.startsWith('#')) {
var index = line.indexOf(':');
var packageName = line.substring(0, index + 1);
var relativePath = line.substring(index + 1);
var absolutePath = path.join(sdkDirPath, relativePath);
// Convert to 'file:///' URI since that's how absolute paths in
// package config files must be for Windows.
// Convert to file:/// URI since that's how absolute paths in
// .packages must be for windows
absolutePath = Uri.file(absolutePath).toString();
buffer.write(packageName);
buffer.writeln(absolutePath);
}
}
return buffer.toString();
}

/// The default content of the plugin. This is a minimal plugin that will only
/// respond correctly to version checks and to shutdown requests.
String _defaultPluginContent() {
return r'''
import 'dart:async';
import 'dart:isolate';
import 'package:analyzer/file_system/file_system.dart';
import 'package:analyzer/file_system/physical_file_system.dart';
import 'package:analyzer/src/dart/analysis/driver.dart';
import 'package:analyzer_plugin/plugin/plugin.dart';
import 'package:analyzer_plugin/protocol/protocol_generated.dart';
import 'package:analyzer_plugin/starter.dart';
import 'package:pub_semver/pub_semver.dart';
void main(List<String> args, SendPort sendPort) {
MinimalPlugin plugin = new MinimalPlugin(PhysicalResourceProvider.INSTANCE);
new ServerPluginStarter(plugin).start(sendPort);
}
class MinimalPlugin extends ServerPlugin {
MinimalPlugin(ResourceProvider provider) : super(provider);
@override
List<String> get fileGlobsToAnalyze => <String>['**/*.dart'];
@override
String get name => 'minimal';
@override
String get version => '0.0.1';
@override
AnalysisDriverGeneric createAnalysisDriver(ContextRoot contextRoot) => null;
@override
Future<AnalysisHandleWatchEventsResult> handleAnalysisHandleWatchEvents(
AnalysisHandleWatchEventsParams parameters) async =>
new AnalysisHandleWatchEventsResult();
@override
bool isCompatibleWith(Version serverVersion) => true;
}
''';
}

/// Return the content to be used for the '.packages' file.
String _getPackagesFileContent() {
var packagesFileContent = _packagesFileContent;
if (packagesFileContent == null) {
var sdkPackagesFile = io.File(_sdkPackagesPath());
var sdkPackageMap = sdkPackagesFile.readAsLinesSync();
packagesFileContent = _packagesFileContent =
_convertPackageMap(path.dirname(sdkPackagesFile.path), sdkPackageMap);
}
return packagesFileContent;
}

/// Return the path to the '.packages' file in the root of the SDK checkout.
String _sdkPackagesPath() {
var packagesPath = io.Platform.script.toFilePath();
while (packagesPath.isNotEmpty &&
path.basename(packagesPath) != 'analysis_server') {
packagesPath = path.dirname(packagesPath);
}
packagesPath = path.dirname(packagesPath);
packagesPath = path.dirname(packagesPath);
return path.join(packagesPath, '.packages');
}
}

class TestNotificationManager implements AbstractNotificationManager {
Expand Down Expand Up @@ -922,17 +910,3 @@ mixin _ContextRoot on ResourceProviderMixin {
);
}
}

extension on PhysicalResourceProvider {
/// Converts the given posix [filePath] to conform to this provider's path
/// context.
String convertPath(String filePath) {
if (pathContext.style == path.windows.style) {
if (filePath.startsWith(path.posix.separator)) {
filePath = r'C:' + filePath;
}
return filePath.replaceAll(path.posix.separator, path.windows.separator);
}
return filePath;
}
}

0 comments on commit aa6b647

Please sign in to comment.