Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid blocking the pipeline while handling refreshLocal during document lifecycle events #2659

Merged

Conversation

testforstephen
Copy link
Contributor

This is a follow-up on #2518 (comment).

As shown in the callstack below, the default refreshLocal implementation uses two locks: a scheduling rule for jobmanager concurrency and a workspace lock for workspace tree modification. The workspace lock is essential to prevent the workspace tree from being corrupted during refresh. We can try setting the scheduling rule to null for refreshLocal in documentLifecycleHandler, since it only refreshes the current Java file and has a limited scope. I tested the new refresh approach with use cases such as creating, renaming and saving a Java file, it works fine.

- Call Stack:
File.refreshLocal(int,IProgressMonitor) File.class:316
Resource.refreshLocal(int,IProgressMonitor) Resource.class:1565
Workspace.prepareOperation(ISchedulingRule,IProgressMonitor)  Workspace.class:2332
WorkManager.checkIn(ISchedulingRule,IProgressMonitor) WorkManager.class:124

- Locks used in refresh operation:
/**
	 * An operation calls this method and it only returns when the operation is
	 * free to run.
	 */
	public void checkIn(ISchedulingRule rule, IProgressMonitor monitor) throws CoreException {
		boolean success = false;
		try {
			if (workspace.isTreeLocked()) {
				String msg = Messages.resources_cannotModify;
				throw new ResourceException(IResourceStatus.WORKSPACE_LOCKED, null, msg, null);
			}
			jobManager.beginRule(rule, monitor);   // scheduling rule check
			lock.acquire();   // workspace lock check
			incrementPreparedOperations();
			success = true;
		} finally {
			//remember if we failed to check in, so we can avoid check out
			if (!success)
				checkInFailed.set(Boolean.TRUE);
		}
	}

Copy link
Contributor

@jdneo jdneo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

According to the implementation,

jobManager.beginRule(rule, monitor);   // scheduling rule check
lock.acquire();   // workspace lock check

Though jobManager.beginRule(rule, monitor); becomes non-blocking, lock.acquire(); will still block multiple requests. So, it should be safe.

@testforstephen testforstephen merged commit af7326d into eclipse-jdtls:master May 18, 2023
@testforstephen testforstephen deleted the jinbo_refreshlocal branch May 18, 2023 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants