Delete client log files when deleting workspace

[snjeza]

Fixes #3349

I can only reproduce the problem when I stop the o.e.jdt.ls.core plugin while debugging Java LS.
The PR deletes the client.log.* files when run Java: Clean the Java Language Server Workspace.

[rgrunber]

This works, but has a possibly unwanted side-effect.

If you activate the command to clean up the language server, it seems to delete the log file that is in use by the starting language client. Would it be possible to move the workspace cleanup prior to

vscode-java/src/extension.ts

Line 114 in 31f7f3f

initializeLogFile(clientLogFile);

?

That way the newly created log file is unaffected by the delete.

[rgrunber]

I tried the following change to your PR :

diff --git a/src/extension.ts b/src/extension.ts
index 938cf62..7d8c1ba 100644
--- a/src/extension.ts
+++ b/src/extension.ts
@@ -110,7 +110,12 @@ export async function activate(context: ExtensionContext): Promise<ExtensionAPI>
        if (!storagePath) {
                storagePath = getTempWorkspace();
        }
+       const workspacePath = path.resolve(`${storagePath}/jdt_ws`);
        clientLogFile = path.join(storagePath, 'client.log');
+       const cleanWorkspaceExists = fs.existsSync(path.join(workspacePath, cleanWorkspaceFileName));
+       if (cleanWorkspaceExists) {
+               deleteClientLog(storagePath);
+       }
        initializeLogFile(clientLogFile);
 
        const telemetryService: Promise<TelemetryService> = Telemetry.startTelemetry(context);
@@ -137,7 +142,6 @@ export async function activate(context: ExtensionContext): Promise<ExtensionAPI>
        }).then(async (requirements) => {
                const triggerFiles = await getTriggerFiles();
                return new Promise<ExtensionAPI>(async (resolve) => {
-                       const workspacePath = path.resolve(`${storagePath}/jdt_ws`);
                        const syntaxServerWorkspacePath = path.resolve(`${storagePath}/ss_ws`);
 
                        let serverMode = getJavaServerMode();
@@ -315,13 +319,11 @@ export async function activate(context: ExtensionContext): Promise<ExtensionAPI>
                                }
                        }));
 
-                       const cleanWorkspaceExists = fs.existsSync(path.join(workspacePath, cleanWorkspaceFileName));
                        if (cleanWorkspaceExists) {
                                const data = {};
                                try {
                                        cleanupLombokCache(context);
                                        deleteDirectory(workspacePath);
-                                       deleteClientLog(storagePath);
                                        deleteDirectory(syntaxServerWorkspacePath);
                                } catch (error) {
                                        data['error'] = getMessage(error);

Let me know if that works. The reason I kept the client log deletion separate from the workspace deletions is because those deletions also trigger the "clean workspace" telemetry event, and since telemetry initialization is done asynchronously, we can't guarantee that event would succeed if we moved that whole "deletion" block up.

If you find a nicer way, I'm open to it.

[rgrunber]

On last thing. We have

vscode-java/src/extension.ts

Lines 255 to 270 in 31f7f3f

	initializationFailedHandler: error => {
	logger.error(`Failed to initialize ${extensionName} due to ${error && error.toString()}`);
	if (error.toString().includes('Connection') && error.toString().includes('disposed')) {
	if (!initFailureReported) {
	apiManager.fireTraceEvent({
	name: "java.client.error.initialization",
	properties: {
	message: error && error.toString(),
	},
	});
	}
	initFailureReported = true;
	return false;
	} else {
	return true;
	}

& https://github.com/redhat-developer/vscode-java/blob/31f7f3f8c0ae0eaa5d39c3d20d080801e92b20dd/src/clientErrorHandler.ts

One should detect failures in establishing a connection and the other can detect errors during client/server communication. If there are many errors being logged (from calling "stop" on the jdt.ls.core bundle), we have the ability to detect them and maybe prevent the client from attempting to re-initialize.

[snjeza]

@rgrunber I have updated the PR.

[rgrunber]

@snjeza , were you able to reproduce the issue. The best I could do is by calling exit() directly in the initialize() of the language server. Even so, we still attempt to restart 5 times, prior to stopping, but at least there's fewer error messages.

[snjeza]

I have reproduced the issue in the following way;

• install the PDE Support extension
• open eclipse.jdt.ls in VS Code
• wait to Java LS download the PDE target
• exit VS Code
• open eclipse.jdt.ls in Eclipse
• launch jdt.ls.socket-stream
• open vscode-java in VS Code
• run Launch Extension JDTLS Client

Java LS stops the org.eclipse.jdt.ls.core plugin at https://github.com/eclipse-jdtls/eclipse.jdt.ls/blob/a31867d0ee8bd1fd348a7ba52469fc35e1da9369/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BundleUtils.java#L168
org.eclipse.osgi.container.ModuleContainer.addDependents(Module, Map<ModuleRevision, ModuleWiring>, Set) adds the o.e.jdt.ls plugin as a dependent plugin.
org.osgi.framework.wiring.FrameworkWiring.refreshBundles(Collection, FrameworkListener...) tries to restart it and throws NPE at https://github.com/eclipse-jdtls/eclipse.jdt.ls/blob/a31867d0ee8bd1fd348a7ba52469fc35e1da9369/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BundleUtils.java#L266

[rgrunber]

It's not reproducing the issue for me. After running Launch Extension JDTLS Client, should the folder opened for the child instance of VS Code contain eclipse.jdt.ls ? It's not causing the issue at all.

[snjeza]

It's not reproducing the issue for me. After running Launch Extension JDTLS Client, should the folder opened for the child instance of VS Code contain eclipse.jdt.ls ? It's not causing the issue at all.

@rgrunber Could you try the following in Eclipse?

• add the o.e.team.core plugin when launching `jdt.ls.socket-stream
• clear the workspace
• set a breakpoint at https://github.com/eclipse-jdtls/eclipse.jdt.ls/blob/a31867d0ee8bd1fd348a7ba52469fc35e1da9369/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JavaLanguageServerPlugin.java#L364

[rgrunber]

Thanks, that's what was needed. I had to add o.e.team.core. I guess this is still a bit different than the other users because in their case, there was a server log file with data, but in this case none is created.

[rgrunber]

Works for me. Feel free to merge when ready.

I think we should keep the original issue open to better understand how this is happening for those other users. Do you think they have some extension on their system that brings in o.e.team.core ?

[snjeza]

o.e.team.core adds o.e.jdt.ls.core as a dependent plugin. They have it or a similar plugin. BundleUtils.refreshBundles tries to refresh o.e.jdt.ls.core and crashes.

[rgrunber]

So org.eclipse.team.core is an optional dependency of org.eclipse.jdt.core. It isn't present by default in our JDT-LS runtime product. Do I have it correct that if some extension introduces org.eclipse.team.core through the bundles setting, then jdt.ls.core will be restarted by the Equinox runtime ?