Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WFCORE-6662] Remove deprecated usage of ServiceController.getValue() by capturing ModelControllerClientFactory ... #5834

Closed
wants to merge 3 commits into from
Closed

[WFCORE-6662] Remove deprecated usage of ServiceController.getValue() by capturing ModelControllerClientFactory ... #5834

wants to merge 3 commits into from

Conversation

yersan
Copy link
Collaborator

@yersan yersan commented Jan 15, 2024

...via ServiceActivator starting the Bootable JAR and Embedded server

This PR follows up on #5824, also built on top of that one.

Jira issue: https://issues.redhat.com/browse/WFCORE-6662

bstansberry and others added 2 commits January 9, 2024 08:01
@bstansberry
[WFCORE-6658] Convert ControlledProcessStateService to the current MS…
1d3bd88 
…C API
@yersan
[WFCORE-6662] Remove deprecated usage of ServiceController.getValue()…
ebe0d12 
… by capturing ModelControllerClientFactory via ServiceActivator starting the Bootable JAR and Embedded server

Jira issue: https://issues.redhat.com/browse/WFCORE-6662
@yersan yersan requested a review from bstansberry January 15, 2024 13:50
@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Jan 15, 2024
@wildfly-ci
Copy link

Core -> Full Integration Build 13403 outcome was FAILURE using a merge of ebe0d12
Summary: Tests failed: 1 (1 new), passed: 7685, ignored: 124 Build time: 03:44:32

Failed tests

org.jboss.as.test.clustering.cluster.ejb.timer.DistributedTimerServiceTestCase.test: java.lang.AssertionError: class org.jboss.as.test.clustering.cluster.ejb.timer.beans.CalendarPersistentTimerBean={node-1=[]}. Actual: 0
	at org.jboss.as.test.clustering.cluster.ejb.timer.AbstractTimerServiceTestCase.test(AbstractTimerServiceTestCase.java:302)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor35.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor34.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor33.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor5.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor4.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor3.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor15.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor14.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor5.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor4.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor3.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
------- Stdout: -------
node-2  [0m15:01:08,201 INFO  [org.jboss.as.repository] (management-handler-thread - 1) WFLYDR0001: Content added at location /opt/buildAgent/work/e8e0dd9c7c4ba60/full/testsuite/integration/clustering/target/wildfly-2/standalone/data/content/91/bcbbcb9e7d8d7ffececc01debb8531ac30fd9e/content
 [0mnode-2  [0m15:01:08,205 INFO  [org.jboss.as.server.deployment] (MSC service thread 1-5) WFLYSRV0027: Starting deployment of "DistributedTimerServiceTestCase.war" (runtime-name: "DistributedTimerServiceTestCase.war")
 [0mnode-2  [0m15:01:08,547 INFO  [org.jboss.weld.deployer] (MSC service thread 1-2) WFLYWELD0003: Processing weld deployment DistributedTimerServiceTestCase.war
 [0mnode-2  [0m15:01:08,558 INFO  [org.jboss.as.ejb3.deployment] (MSC service thread 1-2) WFLYEJB0473: JNDI bindings for session bean named 'AutoTransientTimerBean' in deployment unit 'deployment "DistributedTimerServiceTestCase.war"' are as follows:

	java:global/DistributedTimerServiceTestCase/AutoTransientTimerBean!org.jboss.as.test.clustering.cluster.ejb.timer.beans.TimerBean
	java:app/DistributedTimerServiceTestCase/AutoTransientTimerBean!org.jboss.as.test.clustering.cluster.ejb.timer.beans.TimerBean
	java:module/AutoTransientTimerBean!org.jboss.as.test.clustering.cluster.ejb.timer.beans.TimerBean
	java:global/DistributedTimerServiceTestCase/AutoTransientTimerBean
	java:app/DistributedTimerServiceTestCase/AutoTransientTimerBean
	java:module/AutoTransientTimerBean

 [0mnode-2  [0m15:01:08,559 INFO  [org.jboss.as.ejb3.deployment] (MSC service thread 1-2) WFLYEJB0473: JNDI bindings for session bean named 'SingleActionPersistentTimerBean' in deployment unit 'deployment "DistributedTimerServiceTestCase.war"' are as follows:

	java:global/DistributedTimerServiceTestCase/SingleActionPersistentTimerBean!org.jboss.as.test.clustering.cluster.ejb.timer.beans.ManualTimerBean
	java:app/DistributedTimerServiceTestCase/SingleActionPersistentTimerBean!org.jboss.as.test.clustering.cluster.ejb.timer.beans.ManualTimerBean
	java:module/SingleActionPersistentTimerBean!org.jboss.as.test.clustering.cluster.ejb.timer.beans.ManualTimerBean
	java:global/DistributedTimerServiceTestCase/SingleActionPersistentTimerBean
	java:app/DistributedTimerServiceTestCase/SingleActionPersistentTimerBean
	java:module/SingleActionPersistentTimerBean

 [0mnode-2  [0m15:01:08,559 INFO  [org.jboss.as.ejb3.deployment] (MSC service thread 1-2) WFLYEJB0473: JNDI bindings for session bean named 'AutoPersistentTimerBean' in deployment unit 'deployment "DistributedTimerServiceTestCase.war"' are as follows:

@bstansberry
Copy link
Contributor

@yersan Cool! I filed https://issues.redhat.com/browse/WFCORE-6659 for this and gave it a shot in 303beed, but it failed CI so I shelved it. It looks like yours here works. :)

I'll review it after I get unburied a bit from PTO.

@yersan
Copy link
Collaborator Author

yersan commented Jan 16, 2024

@bstansberry oohh, I missed the https://issues.redhat.com/browse/WFCORE-6659 notification after my PTOs :(

It looks like there is still some stuff to do with this PR, after taking a quick look at your changes, this one is pretty similar, although this one uses the null value on the notifier to establish when the dependent simple services that provide the (ProcessStateNotifier or ModelControllerClientFactory) are up. In your PR, you are using a Future.

The ProcessStateNotifier for the Server and EmbeddedServer is not accessed via the notifierRef when the service that provides its value has not been started yet, but the ModelControllerClientFactory could be accessed under this circumstance on the establishModelControllerClient private method.

This PR also assumes that there are no service reloads in between once the Server and EmbeddedServer have started. That could be an issue, but I think only for the modelControllerClient.

This PR has the issue that it is not correctly cleaning up the clientFactorySvcCaptureRef / notifierRef when the dependent service is down. For the notifierRef I think it is optional, but that could be an issue for clientFactorySvcCaptureRef.

I think I can fix it by using the intermediate service that provides the value of those AtomicReference notifiers, setting them to null if the dependent service goes down. That would require keeping this intermediate service up until the dependent ones are still up.

@yersan yersan marked this pull request as draft January 16, 2024 11:09
@yersan
Copy link
Collaborator Author

yersan commented Jan 16, 2024 •
edited
Loading

Partially resolved, it is still racy between the stop method and getting the values from the notifiers (it was also racy before with the original code where createSuperUserClient could be called when the service was down, I guess there are low probabilities, but that's racy). This version also does not remove the intermediate services as soon as the value from the dependents is received. So, it still needs some work.

bstansberry
bstansberry reviewed Jan 16, 2024
View reviewed changes
bootable-jar/runtime/src/main/java/org/wildfly/core/jar/runtime/Server.java
@@ -80,7 +81,8 @@ private Server(String[] cmdargs, Properties systemProps,
this.systemEnv = systemEnv;
this.moduleLoader = moduleLoader;
this.shutdownHandler = shutdownHandler;
this.clientFactorySvcCaptureRef = new AtomicReference<>(null);
this.clientFactorySvcCaptureRef = new AtomicReference<>();
this.notifierRef = new AtomicReference<>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't need to be a field, as its use is confined to one method and the anonymous Service impl created by a call from that method.

@bstansberry
Copy link
Contributor

The ProcessStateNotifier for the Server and EmbeddedServer is not accessed via the notifierRef when the service that provides its value has not been started yet, but the ModelControllerClientFactory could be accessed under this circumstance on the establishModelControllerClient private method.

I initially did mine without the Future and got test failures because the getModelControllerClient() call in EmbedServerHandler.doHandle would return null. That was because the establishModelControllerClient call in StandaloneServerImpl.start would not create a client, because the clientFactorySvcCaptureRef.get() call would return null. That's what led to my comment at https://issues.redhat.com/browse/WFCORE-6659?focusedId=23738156&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-23738156

@bstansberry
Copy link
Contributor

Partially resolved, it is still racy between the stop method and getting the values from the notifiers (it was also racy before with the original code where createSuperUserClient could be called when the service was down, I guess there are low probabilities, but that's racy). This version also does not remove the intermediate services as soon as the value from the dependents is received. So, it still needs some work.

Yes, the createSuperUserClient call should only happen if the clientFactorySvcCaptureRef.get() doesn't return null.

Note that it's ok for ModelControllerClient use to fail in the middle of a reload. If you create a client to a remote server and someone (including yourself) reloads the server, that client isn't expected to work through that. The CLI uses a client that is designed to handle that. The client we return from getModelControllerClient does as well.

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 2, 2024

There has been no activity on this PR for 45 days. It will be auto-closed after 90 days.

@github-actions github-actions bot added the Stale label Mar 2, 2024
@github-actions GitHub Actions
Copy link

There has been no activity on this PR for 90 days and it has been closed automatically.

@github-actions github-actions bot closed this May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bstansberry bstansberry bstansberry left review comments

Assignees
No one assigned
Labels
deps-ok Dependencies have been checked, and there are no significant changes Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yersan @wildfly-ci @bstansberry