Refactoring request (Kernel32.java)

[dEajL3kA]

In Kernel32Util.java there are many functions that have the following form:

public static final String SomeFunctionName(int pid, int dwFlags) {
    HANDLE hProcess = null;
    Win32Exception we = null;

    try {
        /* ... */
    } catch (Win32Exception e) {
        we = e;
        throw we; // re-throw to invoke finally block
    } finally {
        try {
            closeHandle(hProcess);
        } catch (Win32Exception e) {
            if (we == null) {
                we = e;
            } else {
                we.addSuppressed(e);
            }
        }
        if (we != null) {
            throw we;
        }
    }
}

First of all, I think this line in the catch block is superfluous/redundant:

throw we; // re-throw to invoke finally block

That is because the finally block will be executed, regardless of whether there was an exception or not. Even if there was an exception and if that exception was caught by a catch block, the finally block still is always going to be executed. Also, because of the if (we != null) {throw we} inside the finally block, the exception is guaranteed to be thrown eventually...

Furthermore, because the above pattern occurs many times, I'd suggest refactoring it to:

public static final String SomeFunctionName(int pid, int dwFlags) {
    HANDLE hProcess = null;
    Win32Exception we = null;

    try {
        /* ... */
    } catch (final Win32Exception e) {
        we = e;
    } finally {
        cleanUp(hProcess, we);
     }
}

/* Helper method to encapsulate the clean-up code */
private static void cleanUp(final HANDLE h, Win32Exception we) {
    try {
        closeHandle(h);
    } catch (final Win32Exception e) {
        if (we == null) {
            we = e;
        } else {
            we.addSuppressedReflected(e);
        }
    }
    if (we != null) {
        throw we;
    } 
}

This should eliminate a whole lot of "copy & paste" code 😏

[matthiasblaesing]

My personal feeling: Don't fix it if it is not broken. I'm inclined not to waste time on beautification of code, that noone will touch in the future again.

[dEajL3kA]

But this feels wrong: Currently, when the catch block is triggered, it saves the exception to we (which is okay), but then it also throws the exception. However, because the finally block is always executed, and because the finally block is going to throw the previously saved exception from we, the exception that was thrown from the catch will always be replaced/discarded.

So, unless I'm missing something here, the current "re-throw to invoke finally block" effectively does nothing, because in all possible cases the thrown exception is going to be discarded shortly thereafter by the finally block – but probably the pointless throw still has some runtime overhead; and it certainly makes the code more confusing.

Note:

If the catch block completes abruptly for reason R, then the finally block is executed. Then there is a choice:

• If the finally block completes normally, then the try statement completes abruptly for reason R.
• If the finally block completes abruptly for reason S, then the try statement completes abruptly for reason S (and reason R is discarded).

Also, I would like to refactor the repeating clean-up code into a separate method, because otherwise this pull request would require to add even more "1:1" copies of the very same clean-up code in order to implement proper error handling 🤔

[matthiasblaesing]

If you find someone to review and merge this, fine with me, I will not be that someone.

[dbwiddis]

If you find someone to review and merge this, fine with me, I will not be that someone.

I might consider reviewing this, but I need to be absolutely sure nothing is going to be broken. As @matthiasblaesing said, "Don't fix it if it is not broken." Translated to US Southern Slang, "If it ain't broke, don't fix it.".

Before even starting, I'd like you to explain what happens in JDK 1.6 and what happens later JDKs with this odd situation of both throwing an exception in the catch block and throwing that exact same object in the finally block. Which one actually reaches the user? Did it change with JDK 1.7's exception handling changes?

If you do refactor, ease my review:

1. Separate into commits that all do exactly the same thing so I can quickly identify any errors.
2. If we're refactoring let's use a verbose method name. cleanUp() doesn't tell me anything. closeHandleAndRethrow() is better.
3. Is this pattern only used in Kernel32? If it's used elsewhere, this may change how we choose to refactor.

[dEajL3kA]

Before even starting, I'd like you to explain what happens in JDK 1.6 and what happens later JDKs with this odd situation of both throwing an exception in the catch block and throwing that exact same object in the finally block. Which one actually reaches the user? Did it change with JDK 1.7's exception handling changes?

I don't think it's an implementation choice. It's clear from the language spec that:

1. The finally block is executed, regardless of whether there was an exception in the try block or not.
2. The finally block is executed, even if there was an exception in the try block and that exception was caught by a catch block – regardless of whether the executed catch block throws an exception or not.
3. If the finally block throws an exception, then that exception reaches the user; the previous exception is discarded.

See here:
https://docs.oracle.com/javase/specs/jls/se19/html/jls-14.html#jls-14.20.2

(Note that in this text "completes abruptly" actually means "throws an exception", as opposed to "completes normally")

[dEajL3kA]

Separate into commits that all do exactly the same thing so I can quickly identify any errors.

Like this?
https://github.com/java-native-access/jna/pull/1544/commits

[dEajL3kA]

Before even starting, I'd like you to explain what happens in JDK 1.6 and what happens later JDKs with this odd situation of both throwing an exception in the catch block and throwing that exact same object in the finally block.

Wasn't easy to find a working download of JDK 1.6 (Java 6) in 2023, but here we go!

Consider this example:

public class ExceptionTest {

    public static class MyException extends Exception {

        private static final long serialVersionUID = 1L;
        
        public MyException(final String message) {
            super(message);
        }
    }

    public static void main(String[] args) {
        System.err.println("Running on Java v" + System.getProperty("java.version"));
        try {
            test();
        } catch (final Throwable e ) {
            System.err.println("EXCEPTION:");
            e.printStackTrace();
        }
        System.err.println("goodbye!");
    }

    private static void test() throws MyException {
        MyException err = null;
        try {
            System.err.println("now executing \"try\"");
            throw new MyException("A");
        } catch(MyException e) {
            System.err.println("now executing \"catch\"");
            err = e;
        } finally {
            System.err.println("now executing \"finally\"");
            try {
                throw new MyException("B");
            } catch (MyException e) {
                if (err != null) {
                    //err.addSuppressed(e); <-- not supported by JDK 1.6
                } else {
                    err = e;
                }
            }
            if (err != null) {
                throw err;
            }
        }
    }
}

JDK 1.6 output is exactly as expected, according to the language spec:

Running on Java v1.6.0-119
now executing "try"
now executing "catch"
now executing "finally"
EXCEPTION:
ExceptionTest$MyException: A
	at ExceptionTest.test(ExceptionTest.java:29)
	at ExceptionTest.main(ExceptionTest.java:17)
goodbye!

Note: No throw required in the catch block, in order to make the finally block execute 😏

---

Output from up-to-date JDK 17 is the same:

(except that, with an up-to-date JDK, we can add the "suppressed" exception to the actual exception)

Running on Java v17.0.8
now executing "try"
now executing "catch"
now executing "finally"
EXCEPTION:
ExceptionTest$MyException: A
	at ExceptionTest.test(ExceptionTest.java:29)
	at ExceptionTest.main(ExceptionTest.java:17)
	Suppressed: ExceptionTest$MyException: B
		at ExceptionTest.test(ExceptionTest.java:36)
		... 1 more
goodbye!

[dbwiddis]

3. If the finally block throws an exception, then that exception reaches the user; the previous exception is discarded.

See here: https://docs.oracle.com/javase/specs/jls/se19/html/jls-14.html#jls-14.20.2

Thanks for digging that up. That's what I needed.

Like this?
https://github.com/java-native-access/jna/pull/1544/commits

Precisely.

Looking forward to your PR.

[dblock]

Closed via #1544