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

Fix build block user action (#472) #1743

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

haideryaqoob720
Copy link

@haideryaqoob720 haideryaqoob720 commented Feb 18, 2025

Problem

The Eclipse IDE has a bug where certain user actions (such as saving edits or interacting with version management) are blocked during the build process in the Eclipse IDE, even though they are not dependent on the ongoing compilation. The blocking behavior should be limited to actions that explicitly rely on the build process, such as Run and Debug.

Solution

To refine the selectivity of the blocking behavior, implementations have been made to the identifyThreadAction method within ThreadJob.java. The updated logic ensures that only jobs explicitly related to User actions (such as edit, save, rename or interacting with VCS and so on) are not blocked during the build process.

Changes Implemented

Implemented identifyThreadAction() to distinguish between build-dependent jobs and UI-related threads.
Make sure that user actions aren’t blocked by the build.
For more details, refer to the related bug report:
Eclipse Bug Report #329657
GitHub Eclipse Discussion

Demonstration

A short demo video showcasing the changes in action is available here: demo

Copy link
Member

@iloveeclipse iloveeclipse left a comment

Choose a reason for hiding this comment

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

  1. Please explain the problem and solution strategy in the PR description & commit message.
  2. Using job or thread names is a "NO GO", sorry. We can't make assumptions about job names. Either you use known job families or something else, but matching / guessing names is not acceptable.

@akurtakov
Copy link
Member

Would you please react to the comment here?

@haideryaqoob720
Copy link
Author

Would you please react to the comment here?

I've been working on replacing string-based identifiers with job families

@izubaire
Copy link

Hey @akurtakov @iloveeclipse, I’ve introduced isUI() methods in LockManager and LockListener to detect the UI thread and skip waiting when isUI() is true. This change prevents the UI from getting blocked by background build jobs, keeping interactions responsive for the user.

Issue:
The user’s actions were getting blocked while a build job was running. In other words, the UI thread was waiting for the background build process to complete before allowing user interaction.

What Changed:
A new method, isUI(), has been introduced in both the LockManager and LockListener classes. This method checks if the current thread is the UI thread.
The joinRun method in ThreadJob has been updated to call isUI(). Now, if the thread is identified as the UI thread, it will not be forced to wait for the background build to finish.

How It Solves the Issue:
By detecting whether the calling thread is the UI thread, we ensure that UI interactions are no longer blocked by a long-running build job. This allows the user to continue interacting with the UI while the background build proceeds independently

@eclipse-platform-bot
Copy link
Contributor

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

runtime/bundles/org.eclipse.core.jobs/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch
From b8af36af3fd32836e99faab0d0133ca3e07632bb Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Tue, 18 Mar 2025 17:08:02 +0000
Subject: [PATCH] Version bump(s) for 4.36 stream


diff --git a/runtime/bundles/org.eclipse.core.jobs/META-INF/MANIFEST.MF b/runtime/bundles/org.eclipse.core.jobs/META-INF/MANIFEST.MF
index d3660e7a86..4e6320b6b9 100644
--- a/runtime/bundles/org.eclipse.core.jobs/META-INF/MANIFEST.MF
+++ b/runtime/bundles/org.eclipse.core.jobs/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.core.jobs; singleton:=true
-Bundle-Version: 3.15.500.qualifier
+Bundle-Version: 3.15.600.qualifier
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
 Export-Package: org.eclipse.core.internal.jobs;x-internal:=true,
-- 
2.48.1

Further information are available in Common Build Issues - Missing version increments.

Copy link
Contributor

github-actions bot commented Mar 26, 2025

Test Results

 1 518 files   -  80   1 518 suites   - 80   1h 39m 15s ⏱️ + 8m 45s
 4 020 tests  - 153   3 994 ✅  - 156   23 💤 ±0  3 ❌ +3 
11 542 runs   - 456  11 369 ✅  - 463  166 💤 ±0  7 ❌ +7 

For more details on these failures, see this check.

Results for commit 74ed88c. ± Comparison against base commit 9e339b2.

This pull request removes 153 tests.
AllCompareTests AsyncExecTests ‑ testCancelOnRequeue
AllCompareTests AsyncExecTests ‑ testQueueAdd
AllCompareTests AsyncExecTests ‑ testWorker
AllCompareTests CompareFileRevisionEditorInputTest ‑ testPrepareCompareInputWithNonLocalResourceTypedElements
AllCompareTests CompareUIPluginTest ‑ testFindContentViewerDescriptorForTextType_StreamAccessor
AllCompareTests CompareUIPluginTest ‑ testFindContentViewerDescriptor_TextType_NotStreamAccessor
AllCompareTests CompareUIPluginTest ‑ testFindContentViewerDescriptor_UnknownType
AllCompareTests ContentMergeViewerTest ‑ testFFFX
AllCompareTests ContentMergeViewerTest ‑ testFFTX
AllCompareTests ContentMergeViewerTest ‑ testFFXF
…

♻️ This comment has been updated with latest results.

@haideryaqoob720
Copy link
Author

Hey @iloveeclipse, thanks for the great feedback - pushed the suggested changes a while back.
Big thanks to @akurtakov for the follow-up and to @izubaire for collaborating as part of the team. Would really appreciate it if you could take a look and share your feedback.

@iloveeclipse
Copy link
Member

@haideryaqoob720 :

  1. there are two test failures caused by your patch: https://ci.eclipse.org/platform/job/eclipse.platform/job/PR-1743/6/testReport/
  2. A new test is needed to demonstrate the problem and the fix.

@@ -46,6 +46,12 @@ public boolean aboutToWait(Thread lockOwner) {
return false;
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to document this new API method and explain in details when/why should one say it's UI . Even if it looks "obvious" - having things written explicitly helps in reducing difference in expectations.

@mickaelistria
Copy link
Contributor

The blocking behavior should be limited to actions that explicitly rely on the build process, such as Run and Debug.

I don't think this assertion is true. The strategy to decide whether to block an action or not is to rely on SchedulingRules. If a build job has a rule holding a file, then it's expected that the file being hold cannot be modified.
In general, instead of trying to bend the Jobs framework, a saner and better strategy involves finding the Job that holds the rule causing the blocked operation and then trying to minimize the impact of its scheduling rule (ie not block a whole project, but only block the few files in needs to read, only for the shortest possible time).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants