Bug 563459 - Enhance UISynchronize to make it more useful
Summary: Enhance UISynchronize to make it more useful
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.14   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 4.19 RC1   Edit
Assignee: Christoph Laeubrich CLA
QA Contact:
URL:
Whiteboard:
Keywords: noteworthy
Depends on:
Blocks: 571421
  Show dependency tree
 
Reported: 2020-05-22 02:04 EDT by Christoph Laeubrich CLA
Modified: 2021-02-23 01:34 EST (History)
5 users (show)

See Also:
loskutov: pmc_approved? (mistria)

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Christoph Laeubrich CLA 2020-05-22 02:04:35 EDT 
Currently UISynchronize merely replicates the Methods in Display but lacks for example a way to check if the current Thread is an UI-Thread.

Beside this the API makes it hard to be used as only runnables are allowed. Also for example things like BusyIndicator.showWhile(...) does not work because the require the display.

My proposal is to add at least the following methods to the UISyncronize:

- abstract isUIThread(Thread t);
- isUIThread() --> calling isUIThread with current thread)
- abstract executeBusy(Runnable) (compatible with the BusyIndicator call)

Beside this I created for the chemclipse project a set of functions that has be proven to be very useful and making life alot easier, mybe they can be in-cooperated into the UISynchronize adding a real benefit over using the raw Display class:

- <T> T executeBusy(Callable<T> action) 
- <T> T executeInUserInterfaceThread(Callable<T> action) 
- void executeInUserInterfaceThread(Runnable action) 

especially the first is very useful to execute things that are to short for  progressmonitor dialog but to long to be executed directly in the UIThread but you need the result afterwards.
This is a little bit similar to SwingWorker approach in swing.
Comment 1 Eclipse Genie CLA 2020-12-25 11:25:34 EST 
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/174116
Comment 2 Christoph Laeubrich CLA 2020-12-25 11:31:01 EST 
I have now created an implementation proposal for this, would be good if someone could review and give some comments.
Comment 4 Eclipse Genie CLA 2020-12-30 11:16:19 EST 
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui.tools/+/174150
Comment 6 Andrey Loskutov CLA 2020-12-31 10:23:06 EST 
(In reply to Eclipse Genie from comment #3)
> Gerrit change
> https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/174116 was merged
> to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/
> ?id=7d105cf84b70272ef42aea426dd0c5a2deeafecb

This caused releng test failure due the broken javadoc, see

https://download.eclipse.org/eclipse/downloads/drops4/I20201230-2150/testResults.php

and

https://download.eclipse.org/eclipse/downloads/drops4/I20201230-2150/compilelogs/platform.doc.isv.javadoc.txt

../../../eclipse.platform.ui/bundles/org.eclipse.e4.ui.di/src/org/eclipse/e4/ui/di/UISynchronize.java:137: error: exception not thrown: java.util.concurrent.ExecutionException
	 * @throws ExecutionException   if the concurrent execution has thrown an
	           ^
1 error

Please fix that error.
Comment 7 Christoph Laeubrich CLA 2021-01-01 05:43:23 EST 
(In reply to Andrey Loskutov from comment #6)
> Please fix that error.

Thanks for notice, I'll provide a fix for that, I'm just wondering why such problems are not reported by the gerrit checks can this be enabled for the verification builds to also check the javadoc?
Comment 8 Eclipse Genie CLA 2021-01-01 06:17:46 EST 
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/174168
Comment 10 Lars Vogel CLA 2021-01-06 08:14:12 EST 
Please add to N&N. This is a nice additional to the API.

Also if this bug is fixed, please set the milestone and move it to FIXED.
Comment 11 Christoph Laeubrich CLA 2021-01-08 02:55:08 EST 
(In reply to Lars Vogel from comment #10)
> Please add to N&N. This is a nice additional to the API.
> 
> Also if this bug is fixed, please set the milestone and move it to FIXED.

I'll provide a N&N next week and will update the bug then.
Comment 12 Eclipse Genie CLA 2021-01-11 03:56:58 EST 
New Gerrit change created: https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/174590
Comment 13 Thomas Schindl CLA 2021-01-11 04:00:35 EST 
I know I'm late to the game but I'm not sure it is a good idea to add more syncExec/blocking-APIs because of the danger to run into deadlocks. I would suggest to make those APIs return a CompletableFuture.

If you want to stick with the syncExec-APIs I would suggest to add paragraph to the JavaDoc informing users about the potential problems resulting in the useage of such an API.
Comment 14 Christoph Laeubrich CLA 2021-01-11 04:03:34 EST 
Can you elaborate where you see a dead-lock problem here and how completable future can solve this?
Comment 15 Thomas Schindl CLA 2021-01-12 03:19:58 EST 
---
package test;

import java.util.concurrent.CompletableFuture;

import org.eclipse.swt.SWT;
import org.eclipse.swt.events.SelectionAdapter;
import org.eclipse.swt.events.SelectionEvent;
import org.eclipse.swt.layout.GridLayout;
import org.eclipse.swt.widgets.Button;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Shell;

public class TestShell {
  public static void main(String[] args) {
    Display d = new Display();
    Shell s = new Shell(d);
    s.setLayout(new GridLayout(1, false));
    Button b = new Button(s, SWT.PUSH);
    b.setText("Hello");
    b.addSelectionListener( new SelectionAdapter() {
      @Override
      public void widgetSelected(SelectionEvent e) {
        CompletableFuture.runAsync( () -> {
          d.syncExec( () -> {
            System.err.println("Before");
            String val = CompletableFuture.supplyAsync( () -> {
              d.syncExec( () -> {
                System.err.println("Never reached");
              });
              return "";
            }).join();
            System.err.println("After");
          });  
        });
        
      }
    });
    s.open();
    
    while( ! s.isDisposed() ) {
      if( ! d.readAndDispatch() ) {
        d.sleep();
      }
    }
  }
}
---
Comment 16 Christoph Laeubrich CLA 2021-01-12 03:42:46 EST 
@Thomas but how do you avoid this by using CompletableFuture?

The extension is just meant make it easier to do things right, if SWT allows to create a deadlock in some situations there is probably a way with the new API as well, it just lets people write code that is way less error prone to those situations.

As there is not much context from your code its hard to tell how it would be done better with the new approach...
Comment 17 Thomas Schindl CLA 2021-01-12 05:08:04 EST 
well just because SWT sllows to get people into a deadlock situation (without warning in it its javadoc) is not justification that an upstream project advertises the same programming pattern.

I fully understand that in some situations one has to have blocking code (=syncExec) to implement API contracts but the easier you make it the more people will use this pattern and the bigger the likelyness that you get into a deadlock situation.

A deadlock free pattern can not be a replacement for syncExec (and can never be) but you have to options for async:
* Future/Promise based APIs
* Callback based APIs

where the Callback-based APIs are not as fluent as Future/Promise based ones. To summerize if you just make sync programming easier and make async hard/left behind you'll get more sync code.
Comment 19 Sarika Sinha CLA 2021-02-16 07:56:34 EST 
@Christoph,
Can you move the N&N item to platform_isv as this is API related change.
Comment 20 Eclipse Genie CLA 2021-02-16 08:04:35 EST 
New Gerrit change created: https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/176340
Comment 22 Christoph Laeubrich CLA 2021-02-19 06:36:16 EST 
@Thomas I have played around a bit more with this and think we can replace all sync calls with the following pattern:

  public <T> CompletableFuture<T> call(Callable<T> action) throws InterruptedException, ExecutionException {
    CompletableFuture<T> future = new CompletableFuture<>();
    Thread thread = Thread.currentThread();
    if (isUIThread(thread)) {
      try {
        future.complete(action.call());
      } catch (Exception e) {
        future.completeExceptionally(e);
      }
    } else {
      syncExec(new Runnable() {

        @Override
        public void run() {
          try {
            future.complete(action.call());
          } catch (Exception e) {
            future.completeExceptionally(e);
          }
        }
      });
    }
    return future;
  }


This just slightly changes the way a caller use this but makes it much more flexible, and avoid the sync call on SWT Display.

What do you think? (I hope its not too late already to improve on this :-)
Comment 23 Eclipse Genie CLA 2021-02-19 07:07:01 EST 
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/176516
Comment 24 Christoph Laeubrich CLA 2021-02-19 23:18:41 EST 
As Andrey has suggested in the review it seems better to postpone the public API changes to the next release to get some more time to finally negotiate on this.
Comment 25 Eclipse Genie CLA 2021-02-19 23:23:26 EST 
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/176560
Comment 26 Andrey Loskutov CLA 2021-02-22 03:24:56 EST 
(In reply to Eclipse Genie from comment #25)
> New Gerrit change created:
> https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/176560

Lars, this will remove yet-to-be-discussed API. Please approve for RC1.
Comment 27 Lars Vogel CLA 2021-02-22 04:35:42 EST 
(In reply to Andrey Loskutov from comment #26)
> (In reply to Eclipse Genie from comment #25)
> > New Gerrit change created:
> > https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/176560
> 
> Lars, this will remove yet-to-be-discussed API. Please approve for RC1.

Currently working for a client hence not available. Please find another reviewer.I don't think RC1 require PMC approval.
Comment 28 Andrey Loskutov CLA 2021-02-22 04:41:31 EST 
(In reply to Lars Vogel from comment #27)
> (In reply to Andrey Loskutov from comment #26)
> > (In reply to Eclipse Genie from comment #25)
> > > New Gerrit change created:
> > > https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/176560
> > 
> > Lars, this will remove yet-to-be-discussed API. Please approve for RC1.
> 
> Currently working for a client hence not available. Please find another
> reviewer.I don't think RC1 require PMC approval.

See https://www.eclipse.org/eclipse/development/plans/freeze_plan_4_19.php:

RC1
All fixes submitted must have a project lead or PMC vote on the bug report, and the fix must be reviewed by an additional committer (any committer other than the one who made the fix). A positive review from a project lead or PMC member means implicit approval and no vote is needed on the bug report. Ongoing changes to documentation, tests or examples do not require approval.

@Mickael?
Comment 30 Andrey Loskutov CLA 2021-02-22 05:28:31 EST 
(In reply to Eclipse Genie from comment #29)
> Gerrit change
> https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/176560 was merged
> to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/
> ?id=9d7194d5b61f010662d7992f85119d5738b12b05

Christoph, two things before closing this:
1) Please check if we should update N&N
2) Create a follow up enhancement for 4.20 release.
Comment 31 Eclipse Genie CLA 2021-02-23 00:09:02 EST 
New Gerrit change created: https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/176715