Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 336010

Summary: SyncExec in XTextEditorErrorTickUpdater causes hangs in workbench
Product: [Modeling] TMF Reporter: Samantha Chan <chanskw>
Component: XtextAssignee: Project Inbox <tmf.xtext-inbox>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: knut.wannheden, sebastian.zarnekow
Version: 1.0.1Flags: sebastian.zarnekow: helios+
sebastian.zarnekow: indigo+
Target Milestone: M6   
Hardware: PC   
OS: Windows XP   
Whiteboard:
Attachments:
Description Flags
Updated XTextEditorErrorTickUpdater none

Description Samantha Chan CLA 2011-02-01 14:38:30 EST 
In the UpdateEditorImageJob, there is a UI.syncExec call to update the editor title image.

In my understanding, we should avoid using UI.syncExec as much as possible, as it introduces risk to deadlock.  I am wondering why we need to use sync exec in the UpdateEditorImageJob.

Here's the scenario where my product runs into deadlock because of this sync exec:
1)  In the workbench, move a file from one folder to another folder using drag and drop
2)  This causes us to spawn off a job to build the project.  Part of the build and processing happens on the UI thread.
3)  This processing causes new error markers to be created on the file being moved.
4)  The new error marker causes the UpdateEditorImageJob to get run, which spawns off a syncexec onto the UI thread.
5)  However, because we are still doing processing on the UI thread, the UpdateEditorImpageJob can never run and finish.
6)  Our processing can never finish because we are waiting for the UpdateEditorImageJob to complete.
Comment 1 Sebastian Zarnekow CLA 2011-02-01 14:56:47 EST 
Hi Samantha,

the syncExec is in an own job which is scheduled independently from the originating event. How does it prevent the other tasks from being finished?
Comment 2 Samantha Chan CLA 2011-02-01 15:12:02 EST 
(In reply to comment #1)
> Hi Samantha,
> 
> the syncExec is in an own job which is scheduled independently from the
> originating event. How does it prevent the other tasks from being finished?

You are right... I am not able to consistently reproduce this, so I cannot see it now.  But when I saw the deadlock, the UI thread was busy doing something, possibly reporting status on a user job that we spawned off.  The updater is trying to get a hold of the UI thread, but could not.  For some reason, because this update job cannot finish, our user job can never finish.

I will dig a bit deeper to be more clear.  But is there any reason why we need a sync exec here?  It seems to me that we can just create a UI job and put a scheduling rule on that to update the editor title image... Why create a job that runs on syncExec?
Comment 3 Sebastian Zarnekow CLA 2011-02-01 15:15:29 EST 
Your are right, using a UI Job would simplify the code. Could you attach a patch?
Comment 4 Samantha Chan CLA 2011-02-01 17:19:15 EST 
(In reply to comment #3)
> Your are right, using a UI Job would simplify the code. Could you attach a
> patch?

sure, let me try to create a patch...
Comment 5 Knut Wannheden CLA 2011-02-02 00:50:06 EST 
(In reply to comment #2)
> (In reply to comment #1)
> > Hi Samantha,
> > 
> > the syncExec is in an own job which is scheduled independently from the
> > originating event. How does it prevent the other tasks from being finished?
> 
> You are right... I am not able to consistently reproduce this, so I cannot see
> it now.  But when I saw the deadlock, the UI thread was busy doing something,
> possibly reporting status on a user job that we spawned off.  The updater is
> trying to get a hold of the UI thread, but could not.  For some reason, because
> this update job cannot finish, our user job can never finish.
> 
> I will dig a bit deeper to be more clear.  But is there any reason why we need
> a sync exec here?  It seems to me that we can just create a UI job and put a
> scheduling rule on that to update the editor title image... Why create a job
> that runs on syncExec?

How about just using asyncExec instead?
Comment 6 Samantha Chan CLA 2011-02-02 11:58:33 EST 
Created attachment 188166 [details]
Updated XTextEditorErrorTickUpdater

I think both asycnExec and UIJob will work in this case.  Using a UIJob will make the code a bit simpler, as we do not need to run a job, and then schedule the actual UI update onto the UI thread.

sorry, I actually do not have the env set up to create a patch.
I have attached the updated XTextEditorErrorTickUpdater.  I have simply change the updater job to an UIJob, and simplified the code.
Comment 7 Sebastian Zarnekow CLA 2011-02-07 04:44:44 EST 
Pushed fix to Helios Maintenance.
Comment 8 Sebastian Zarnekow CLA 2011-02-26 07:32:08 EST 
see last comment
Comment 9 Karsten Thoms CLA 2017-09-19 17:21:10 EDT 
Closing all bugs that were set to RESOLVED before Neon.0
Comment 10 Karsten Thoms CLA 2017-09-19 17:32:29 EDT 
Closing all bugs that were set to RESOLVED before Neon.0