Re: [cu-dev] [glassfish-dev] Omniconcurrent

[arjan tijms <arjan.tijms@xxxxxxxxx>]

Hi,

The PR indeed had the unfortunate wrong name at first, which was my mistake. Apologies again for that.

We can go ahead with porting the changes directly to GF. Easiest might be here to base that off of Omniconcurrent, and "just" copy that into GF with the correct license.

Both the initial PR (with the code copied/ported into GlassFish) as the second PR (with the code copied to a separate lib) work to a large degree, but did have some 18 failures still.

One was due to the jakarta concurrent api lib not being added to the JSP servlet's class path (in default-web.xml), so a .jsp using the jakarta.concurrent package would not compile. It's not really clear why Payara wouldn't need that, but perhaps there's an additional classpath used there now.

Another one seems to be due to a timing / ordering issue and seems like a bug in the Payara code. It concerns this one:

@Service
@Singleton
public class ConcurrentRuntime implements PostConstruct, PreDestroy {
   
   private static ConcurrentRuntime _runtime;

    public static ConcurrentRuntime getRuntime() {
            if (_runtime == null) {
                throw new RuntimeException("ConcurrentRuntime not initialized");
            }
       
        return _runtime;
    }

    private static void setRuntime(ConcurrentRuntime runtime) {
        _runtime = runtime;
    }

    /**
     * Constructor should be private to follow singleton pattern, but package access for unit testing.
     */
    ConcurrentRuntime() {
        setRuntime(this);
    }
}

The static variable depends on ConcurrentRuntime "accidentally" being injected somewhere. If that happens, a later call to getRuntime() returns the instance. If by chance getRuntime() is called without any prior injection processed, there's an exception. When running the TCK in a certain order it throws the exception. When running the same tests in a slightly different order, it doesn't.

I quickfixed this by means of the following code. I guess that would be useful for Payara as well:

public static ConcurrentRuntime getRuntime() {
        if (_runtime == null) {
            _runtime = Globals.get(ConcurrentRuntime.class);
            if (_runtime == null) {
                throw new RuntimeException("ConcurrentRuntime not initialized");
            }
        }
        return _runtime;
    }

Another issue concerns the context service and this code:

private ContextServiceImpl prepareContextService(String contextServiceJndiName, String contextInfo, boolean contextInfoEnabled, boolean cleanupTransaction) {
        ContextServiceImpl contextService = contextServiceMap.get(contextServiceJndiName);
        if (contextService == null) {

            // ...
            contextService = createContextServiceImpl(contextServiceJndiName, contextInfoEnabled, propagated, cleared, Collections.EMPTY_SET);
            contextServiceMap.put(contextServiceJndiName, contextService);
        }
        return contextService;
    }

Here a default context service is created in a circumstance where a configured one should have been present and the following method should have been called:

public synchronized ContextServiceImpl createContextService(ResourceInfo resource, ContextServiceConfig config) {
        ContextServiceImpl contextService = createContextServiceImpl(config.getJndiName(),
                config.isContextInfoEnabledBoolean(), config.getPropagatedContexts(),
                config.getClearedContexts(), config.getUchangedContexts());
        return contextService;
    }

In general the relation between how deployment descriptor parsing, annotation processing and JNDI entries work together is a little complicated. For that fact alone maybe some restructuring would be beneficial later, moving some of it to the Concurrency RI project for clarity. Some of it is also due to how JNDI based services work in Jakarta EE. Despite volumes of specifications, some of the basic APIs required have not been standardized, and perhaps the choice of making them JNDI based services/resources instead of CDI based wasn't ideal.

I'm attaching a quick 'n dirty drawing how some of the artefacts involves in GF/Payara relate:

It might be good to eventually have some of those responsibilities in the concurrent implementation project instead of as "glue code" in the server.

Kind regards,

Arjan Tijms

On Tue, Jun 21, 2022 at 10:59 AM Steve Millidge (Payara) <steve.millidge@xxxxxxxxxxx> wrote:

Hi Arjan,

 

I emailed in response to the broad request for permission to relicense Payara code that for correct IP flows we would backport the changes. I just noticed the PR to incorporate Omniconcurrent initially was titled “Implement Jakarta Security glue code” until yesterday and has a whole raft of changes to different code in it, probably why I missed it.

 

So are we still doing the work to port the Payara changes or is the alternative working?

 

Steve

 

 

 

From: glassfish-dev <glassfish-dev-bounces@xxxxxxxxxxx> On Behalf Of arjan tijms
Sent: 21 June 2022 09:36
To: glassfish developer discussions <glassfish-dev@xxxxxxxxxxx>
Subject: Re: [glassfish-dev] Omniconcurrent

 

Hi Steve,

 

Please accept my humble apologies for if anything wasn't clear. I was just about to write an email to the list, but it's been hectic with so many things going on and asking for attention.

 

There has been no duplicate work luckily. The OmniConcurrent lib is just the alternative approach I mentioned I was working on during the platform call. The PR had been open for 3 days, and a diverse set of reviewers from different companies was tagged. 

 

The other PR with the Payara code was open for even longer (8 days) and didn't get any response. I think both the PRs and the mailing list are important vehicles for discussion, so the PR could have solicited at least some discussion. That PR was ready to go in, no duplication of effort was required by the Payara team. See https://github.com/eclipse-ee4j/glassfish/pull/23989  It could either be given permission, or copied fully and then sent back as a PR (so that the PR would be fully under Payara's name). I sent an e-mail about that to Payara, suggesting exactly that 9 days ago.

 

In the discussion on the platform mailing list the ask to give permission for much of the glue code didn't get much of a response either, and the much smaller PR for just the CDI extension (that would benefit Concurrency RI anyway, even without GlassFish in the equation) already seemed somewhat controversial.

 

That all said, as mentioned there's no duplication really, as it would still be better to have all the code directly in GlassFish under the EPL. The OmniConcurrent lib is just the backup solution here. It could be just enough to get to ballot, and then at a time and pace that is convenient for everyone it can be replaced. Additionally note that the appserver/concurrent package has not been deleted from GlassFish. This was done exactly for this reason.

 

Hope this all makes it more clear.

 

Kind regards,

Arjan Tijms

 

 

 

 

 

 

 

 

 

 

On Mon, Jun 20, 2022 at 11:48 PM Steve Millidge (Payara) <steve.millidge@xxxxxxxxxxx> wrote:

I've seen a PR bringing in "Omniconcurrent" which seems to contain bits of Payara code. There has been zero discussion on this list of what that is or that that was an approach we were following. Last I heard Payara team were going to backport Payara code to GlassFish. If that isn't the case Payara team can stop duplicating effort and get on with something else.

 

Can people communicate on this list on major dev decisions impacting others.

 

Steve

 

Get Outlook for Android

_______________________________________________
glassfish-dev mailing list
glassfish-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/glassfish-dev

_______________________________________________
glassfish-dev mailing list
glassfish-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/glassfish-dev