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

Investigate JVM signal handling functions #54

Closed
mgaudet opened this issue Sep 18, 2017 · 32 comments
Closed

Investigate JVM signal handling functions #54

mgaudet opened this issue Sep 18, 2017 · 32 comments

Comments

@mgaudet
Copy link
Contributor

mgaudet commented Sep 18, 2017

The implementations of all 3 Signal JVM functions:

  • JVM_RegisterSignal
  • JVM_FindSignal
  • JVM_RaiseSignal

Are questionable. All three need to be validated and completed.

@DanHeidinga
Copy link
Member

The implementations of all 3 Signal JVM functions:

  • JVM_RegisterSignal
  • JVM_FindSignal
  • JVM_RaiseSignal

Are questionable. All three need to be validated and completed. My vague understanding is that J9 relies on patches to the JCL code to work around the need for these 3 functions.

@mgaudet
Copy link
Contributor Author

mgaudet commented Sep 18, 2017

I'll re-title this as "Investigate JVM signal handling", as this came from me looking at #40

@mgaudet mgaudet changed the title Change comment on JVM_FindSignal Investigate JVM signal handling functions Sep 18, 2017
@mstoodle
Copy link
Contributor

I marked as "enhancement" but if "bug" becomes more appropriate we should update it :)

@pshipton
Copy link
Member

@babsingh is this resolved by #283?

@babsingh
Copy link
Contributor

@pshipton no. more changes will be required to complete this. currently, the following scenario is not covered.

public class SigIntTest extends Thread {

        private static Object monitor = new Object();

        public static void main(String args[]) {
                Runtime.getRuntime().addShutdownHook(new SigIntTest());
                System.out.println("Press CTRL-C to exit - Sends SIGINT signal");

                synchronized (monitor) {
                        try {
                                monitor.wait();
                        } catch (InterruptedException e) {
                                System.out.println("Main thread interrupted");
                        }
                        System.out.println("Terminating main thread correctly");
                }
        }

        public void run() {
                System.out.println("OK ... exiting");
                synchronized (monitor) {
                        monitor.notify();
                }
        }

}

Current behavior:

Press CTRL-C to exit
^C

Expected behavior:

Press CTRL-C to exit
^COK... exiting
Terminating main thread correctly

@DanHeidinga
Copy link
Member

Current approach is eclipse-omr/omr#2332

@pshipton
Copy link
Member

pshipton commented May 3, 2018

The initial changes are delivered to enable signal handling on Windows, Linux, and AIX. There will be more internal changes but the shutdown hook functionality should be working as of now.

Linux/AIX eclipse-omr/omr#2494 #1603 #1642
Windows #1816 not the full functionality but enough for CTRL-C to trigger the shutdown hooks

@babsingh
Copy link
Contributor

babsingh commented May 3, 2018

Adding @ibm-rtvs

@pshipton
Copy link
Member

pshipton commented May 4, 2018

The nightly builds from May 4th https://adoptopenjdk.net/nightly.html?variant=openjdk8-openj9 should have the shutdown hook updates.

@SueChaplain
Copy link
Contributor

@pshipton is there more testing to do here or can we remove this issue from the release notes in 0.9.0 please?

@pshipton
Copy link
Member

There are more changes to be delivered before signal handling is fully functional, such as #1832

@pshipton
Copy link
Member

#1832 is merged.

@pshipton
Copy link
Member

pshipton commented May 24, 2018

fyi #1984, it seems signals are not working as well as hoped.

edit: this is merged.

dhong44 pushed a commit to dhong44/openj9 that referenced this issue May 31, 2018
…0-revert-49-revert-48-ibm_sdk

Enable atomic-free JNI on select platforms
@pshipton
Copy link
Member

pshipton commented Jun 1, 2018

Another fix here eclipse-omr/omr#2611

@pshipton pshipton modified the milestones: Release 0.9.0, Release 0.10.0 Jun 1, 2018
@thjaeckle
Copy link

Oh noo, the fix won't be in 0.9.0?

@pshipton
Copy link
Member

pshipton commented Jun 5, 2018

Oh noo, the fix won't be in 0.9.0?

The eclipse-omr/omr#2611 fix (and everything previous) is delivered and included in 0.9.0. I temporarily used this Issue in the 0.9.0 milestone because I couldn't add the omr issue directly, and was too lazy to create another issue in OpenJ9 to track it. Now that its been merged, any remaining work on signals is deferred until the next release. See the details in the release notes #1996

@SueChaplain
Copy link
Contributor

@pshipton The OMR change eclipse-omr/omr#2908 is merged. Presumably there is something else that needs merging to prevents us declaring that signal handling is fixed in 0.10.0? Think it was just Windows that was remaining? Thanks

@pshipton
Copy link
Member

pshipton commented Oct 3, 2018

@SueChaplain There is some work remaining to support other signals. As macOS is taking priority, I've moved this item to the 0.12.0 milestone.

@pshipton
Copy link
Member

pshipton commented Oct 3, 2018

@SueChaplain I believe we can remove the Windows limitation from the 0.10.0 release notes. @babsingh please confirm. See the 0.10.0 release notes #2766

"Full support for shutdown signals is pending on Windows; signal handlers cannot be registered by using sun.misc.Signal or dk.internal.misc.Signal."

@babsingh
Copy link
Contributor

babsingh commented Oct 3, 2018

@SueChaplain @pshipton Peter is correct. Windows has fewer signals. Current implementation supports all signals on Windows. Updated doc:

Currently, shutdown signals (SIGINT, SIGHUP and SIGTERM) and SIGCONT are fully supported on Unix platforms (pLinux, zLinux, xLinux, AIX and zOS). Support for other POSIX signals is pending. See SunMiscSignalTest.java for the list of signals that need to be supported.

@SueChaplain
Copy link
Contributor

@babsingh Thanks. I've updated the release notes for 0.10.0 accordingly.

@pdbain-ibm
Copy link
Contributor

Typo above: "using sun.misc.Signal or dk.internal.misc.Signal." should be jdk?

@pshipton
Copy link
Member

pshipton commented Oct 3, 2018

@pdbain-ibm that line was removed so the typo is gone.
#2766

@SueChaplain
Copy link
Contributor

@pshipton - I think I've lost track of what's remaining here. Can you confirm and let me know the release target please.

@pshipton
Copy link
Member

@SueChaplain there is no real target to complete this work. I'll put it into the 0.15 milestone so we don't loose track of it, but don't be surprised if it moves out further. The release notes should stay as-is.

@babsingh
Copy link
Contributor

This issue can be closed after eclipse-omr/omr#3921 and #5923 are merged. Performing final cross-platform testing.

fyi - @DanHeidinga @pshipton

@pshipton
Copy link
Member

@babsingh please create the doc issue to update the release notes, and then we can close this.

@pshipton
Copy link
Member

There is a docs issue eclipse-openj9/openj9-docs#296, this can be closed.

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

No branches or pull requests

8 participants