eclipse-platform / eclipse.platform.text

8 stars 45 forks source link

Use a custom ForkJoinWorkerThreadFactory in AsyncCompletionProposalPopup #185

Closed Christopher-Hermann closed 1 year ago

Christopher-Hermann commented 1 year ago

What it does

CompletableFuture.supplyAsync() is using the defaultForkJoinWorkerThreadFactory. In case a SecurityManager is installed, using the defaultForkJoinWorkerThreadFactory would result in worker threads with no permissions, which leads to not content assist in the java editor.

Use a custom ForkJoinWorkerThreadFactory implementation to work around this.

See also: https://github.com/eclipse-platform/eclipse.platform/issues/294 https://github.com/eclipse-platform/eclipse.platform/pull/295

How to test

  1. Have any Eclipse-plugin that installs a SecurityManager. Use attached demo plugin foo.bar.securitymanager.zip (Main Menu SecurityManager -> Install SecurityManager) or the following snippet:
    Policy.getPolicy(); // init
    Policy.setPolicy(new Policy() {
    @Override
    public boolean implies(ProtectionDomain domain, Permission permission) {
        return true;
    }
    });
    System.setSecurityManager(new SecurityManager());
  2. Open a Java project and use the content assist (CTRL+space)
  3. The content assist will not work
mickaelistria commented 1 year ago

SecurityManager is deprecated, so I'm a bit worried about bending the internals of Eclipse Platform to bbetter support it. It would seem more interesting that you try to get rid of SecurityManager in your plugins.

merks commented 1 year ago

I tend to agree with @mickaelistria in that I think we should just assume that no one installs a security manager because pretty soon they won't be able to do so at all and anything we add now to accommodate that will be pointless or need to be ripped out...

sratz commented 1 year ago

I tend to agree with @mickaelistria in that I think we should just assume that no one installs a security manager because pretty soon they won't be able to do so at all and anything we add now to accommodate that will be pointless or need to be ripped out...

We do observe security managers being installed by plugins, so this is definitely relevant. Also, in Java 17, it is only deprecated and issues a warning. But it's still possible to install a SecurityManager. Hence, the only point in time where we can say for sure that one cannot be installed anymore, would be after Java 21 is the minimum required version by the Platform.

Up until then we sadly have to assume that everything that's possible will somehow be done. I think therefore we should try to be as compatible as possible.

The code is not introducing much higher complexity and this workaround in the two resolved issues linked https://github.com/eclipse-platform/eclipse.platform/issues/294 https://github.com/eclipse-platform/eclipse.platform/pull/295 has already helped us and our customers a lot.

merks commented 1 year ago

@sratz

Given you folks encountered real such cases and given you are also helping to maintain Eclipse, I certainly will not block such an effort!

In EMF too I am now stuck with a bunch of deprecation warnings around this stuff without really knowing what to do about them, nor even knowing if anyone is relying on such things. I know WebSphere used to rely on these things long, long ago...

sratz commented 1 year ago

In EMF too I am now stuck with a bunch of deprecation warnings around this stuff without really knowing what to do about them, nor even knowing if anyone is relying on such things. I know WebSphere used to rely on these things long, long ago...

Yes, it's quite annoying.

Let's hope that JEP 411 goes through as planned and we can get rid of all this stuff with Java 21 for good.

If there are no objections, I'd propose to merge this change for 4.28.