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.

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

PAYARA-3344 Added System Property for Shutdown Grace Period #3702

Merged
merged 4 commits into from
Feb 12, 2019

Conversation

jbee
Copy link
Contributor

@jbee jbee commented Feb 7, 2019

Initially the plan was to use domain.xml based configuration for the grace period setting. I looked into it but it turned out to be overly complicated due to module dependencies and access limitations. So I went for the simpler option using System property fish.payara.shutdowngrace instead. The period is given is milliseconds. E.g. -Dfish.payara.shutdowngrace=1000 for 1 second grace period.

After a first look into where to add the waiting it seamed most appropriate on the outermost level, like the PayaraMicroImpl#shutdown and equivalent methods. After deeper investigation on possible paths during shutdown I found that the common abstraction to server, micro and embedded that is called independent on how the shutdown was initiated should be the GlassFish abstraction. While there are a handful of implementations some of them are wrappers that delegate or extensions that extend. I think there are only 2 relevant implementations: GlassFishImpl and MicroGlassFish. As MicroGlassFish has access to GlassFishImpl it seamed a good way to share the code by making it a static method in GlassFishImpl. Surely no ideal place for common code but I could not find a better place in a common utility class. I believe that this modification includes embedded as embedded seams to wrap one of the other implementations.

In case of thread interruption or illegal value the grace period is zero and the shutdown continues as usual.

While testing the feature for payara micro I got exceptions logged during or after the grade period caused by further calls to stop/dispose as those were not synchronized for micro.

Cjava.lang.IllegalStateException: Already in STOPPING state.
	at fish.payara.micro.boot.runtime.MicroGlassFish.stop(MicroGlassFish.java:80)
	at fish.payara.micro.boot.runtime.MicroGlassFish.dispose(MicroGlassFish.java:96)
	at fish.payara.micro.impl.PayaraMicroImpl.bootStrap(PayaraMicroImpl.java:1045)
	at fish.payara.micro.impl.PayaraMicroImpl.main(PayaraMicroImpl.java:200)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at fish.payara.micro.boot.loader.MainMethodRunner.run(MainMethodRunner.java:48)
	at fish.payara.micro.boot.loader.Launcher.launch(Launcher.java:107)
	at fish.payara.micro.boot.loader.Launcher.launch(Launcher.java:70)
	at fish.payara.micro.boot.PayaraMicroLauncher.main(PayaraMicroLauncher.java:79)
	at fish.payara.micro.PayaraMicro.main(PayaraMicro.java:397)
java.lang.NullPointerException
	at fish.payara.micro.boot.runtime.MicroGlassFish.stop(MicroGlassFish.java:85)
	at fish.payara.micro.boot.runtime.MicroGlassFish.dispose(MicroGlassFish.java:96)
	at fish.payara.micro.impl.PayaraMicroImpl$1.run(PayaraMicroImpl.java:1600)

Since the GlassFishImpl does synchronize these methods it seams logical that micro should do this as well. I applied it and tested it and it seams to prevent the problem.

As requested I also added a command line option for payara micro: --shutdowngrace <duration-ms>.

@jbee jbee added this to the 5.191 milestone Feb 7, 2019
@jbee jbee self-assigned this Feb 7, 2019
@jbee jbee requested review from smillidge and Pandrex247 February 7, 2019 15:03
if (status == Status.STOPPED || status == Status.STOPPING || status == Status.DISPOSED) {
throw new IllegalStateException("Already in " + status + " state.");
}


status = Status.STOPPING;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question is: after status is STOPPING will this prevent accepting new requests? I think the intention is to finish the ongoing ones but I guess we do not want to accept new ones.

Copy link
Member

Choose a reason for hiding this comment

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

I don't follow what the question is here?
There's a check literally above this that prints an error if multiple things try to stop the server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I wanted to point out is that before the wait only the status was changed. No internal processing has been signalled to stop by that point. I would imagine that it could very well be the case that new request still are accepted and start to process. If the goal with this feature is to give time to finish started requests it seams only consequent not to accept new ones. I would assume that some parts of the server needs to be stopped or flagged to get this behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

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

Searching the source I don't see that status is used much other than to print it into the admin console. Perhaps we could use this status variable better to stop processing etc.However that would be a different JIRA.

@@ -87,10 +90,24 @@ public synchronized void stop() throws GlassFishException {
throw new IllegalStateException("Already in " + status + " state.");
}
status = Status.STOPPING;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here: do we prevent accepting new requests effectively?

@jbee
Copy link
Contributor Author

jbee commented Feb 7, 2019

jenkins test please

@jbee
Copy link
Contributor Author

jbee commented Feb 7, 2019

jenkins test please

@jbee
Copy link
Contributor Author

jbee commented Feb 7, 2019

jenkins test please

@jbee
Copy link
Contributor Author

jbee commented Feb 7, 2019

jenkins test please

if (status == Status.STOPPED || status == Status.STOPPING || status == Status.DISPOSED) {
throw new IllegalStateException("Already in " + status + " state.");
}


status = Status.STOPPING;
Copy link
Member

Choose a reason for hiding this comment

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

I don't follow what the question is here?
There's a check literally above this that prints an error if multiple things try to stop the server.

Copy link
Contributor

@smillidge smillidge left a comment

Choose a reason for hiding this comment

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

It would be nice to have a command line flag for Payara Micro that just set this system property

if (status == Status.STOPPED || status == Status.STOPPING || status == Status.DISPOSED) {
throw new IllegalStateException("Already in " + status + " state.");
}


status = Status.STOPPING;
Copy link
Contributor

Choose a reason for hiding this comment

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

Searching the source I don't see that status is used much other than to print it into the admin console. Perhaps we could use this status variable better to stop processing etc.However that would be a different JIRA.

@jbee
Copy link
Contributor Author

jbee commented Feb 8, 2019

It would be nice to have a command line flag for Payara Micro that just set this system property

I'll add one.

@jbee
Copy link
Contributor Author

jbee commented Feb 8, 2019

jenkins test please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants