My concern is clean IP flows and ensuring the Member Committer agreement is complied with. I propose we make an initial PR with the Payara IP, once that is merged then the IP is donated to the project
under Payara’s member committer agreement. After that is done anybody can work on getting it through the TCK.
Steve
From: cu-dev <cu-dev-bounces@xxxxxxxxxxx>
On Behalf Of arjan tijms
Sent: 21 June 2022 13:37
To: glassfish developer discussions <glassfish-dev@xxxxxxxxxxx>
Cc: cu developer discussions <cu-dev@xxxxxxxxxxx>
Subject: Re: [cu-dev] [glassfish-dev] Omniconcurrent
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.
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
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.
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
_______________________________________________
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
|
