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

Improve GlobalScreen #251

Open
wants to merge 1 commit into
base: 2.2
Choose a base branch
from
Open

Improve GlobalScreen #251

wants to merge 1 commit into from

Conversation

xeruf
Copy link

@xeruf xeruf commented Jan 8, 2019

I was not yet able to test the integrity of these changes due to #250.

@sghpjuikit
Copy link

The boolean flag may need to be volatile.

@xeruf
Copy link
Author

xeruf commented Jan 9, 2019

Actually, if that is necessary, then it has to be atomic

@@ -50,7 +51,13 @@
/**
* Logging service for the native library.
*/
protected static Logger log = Logger.getLogger(GlobalScreen.class.getPackage().getName());
protected static Logger logger = Logger.getLogger(GlobalScreen.class.getPackage().getName());
static {
Copy link
Owner

Choose a reason for hiding this comment

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

so my big concern with this is will it handle the standard Java LogManager properties? See https://docs.oracle.com/cd/E17277_02/html/GettingStartedGuide/managelogging.html#logginglevels

Most of the people complaining about this have no idea how Java's logging facility works.

Copy link
Author

Choose a reason for hiding this comment

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

Because it isn't good. I have already mentioned slf4j, but it is your choice whether you want to use it. Otherwise, there should be a way of checking these properties, but as long as #250 is unresolved I can hardly test it.

@kwhat
Copy link
Owner

kwhat commented Jan 9, 2019

Why do we need a boolean flag? You know for #249, you can override the library loading by using the NativeLibraryLocator inteface. See: https://github.com/kwhat/jnativehook/blob/2.1/src/java/org/jnativehook/NativeLibraryLocator.java

I am still not sure what issue you are trying to solve with this... If the native library is not loaded, statically or via java.library.path property, you will still get undefined method exceptions because the methods declared with the native keyword will not be found.

@xeruf
Copy link
Author

xeruf commented Jan 10, 2019

The problem was that we had a bug where, as soon as the JNativeHook JNA libraries were loaded on Linux, the JavaFX FileChooser crashed. Don't ask me how this worked, but that's how it was. This problem is gone now, but if there is a possibility of breaking something, it should only execute once you actually register, not when the class is accessed in any way. That the native functions aren't defined until then shouldn't be the problem, since registering is afaik the first thing you have to do anyways.
I am not very keen on JNA, but this seemed to me like a proper solution to this.

The boolean flag is for not loading the library twice if it is registered twice. Btw @sghpjuikit it doesn't have to be volatile, since it is only ever accessed from one thread.

@kwhat
Copy link
Owner

kwhat commented Jan 10, 2019

but if there is a possibility of breaking something, it should only execute once you actually register, not when the class is accessed in any way.

You missed the point, as soon as the ClassLoader loads the GlobalScreen these native methods must be defined. You cannot wait until the class is loaded and a method is executed. You really should be using the java.library.path or use your own network based NativeLibraryLocator. I will repeat, you CANNOT load the class with undefined native methods. you will get a java.lang.UnsatisfiedLinkError: Native method not found: exception right after the static {} context is initialized.

Because it isn't good. I have already mentioned slf4j

Why isn't it good? slf4j isn't a bad idea, but it does introduce another run-time dependency. Log4j is however out of the question.

I am not very keen on JNA

This library is written in JNI, not JNA. Yes, JNI/JNA is really the only way to make this work.

@xeruf
Copy link
Author

xeruf commented Jan 10, 2019

Okay, I will revert the initialisation move.

You do know that slf4j is only the adapter, with log4j being one binding to it? In a library you should always take slf4j over log4j.

@sghpjuikit
Copy link

sghpjuikit commented Jan 10, 2019

@kwhat I work with @Xerus2000 on the issue. Our 'problem' was that we needed to avoid loading GlobalScreen.class because it would immediately load some native library that in turn caused problematic behavior on the user system. Hence we tried to isolate the entire library by hiding it behind a facade that loaded lazily on demand, which we had under control (i.e., could refuse to serve on affected platforms).

Personally, I had no idea the native library had to be loaded in static initialization block (I believe you that it must). So we thought that it would be a good idea to try to put the on-demand loading into this very library - now it seems to be an impossible endeavor. And I do not see how java.library.path or NativeLibraryLocator can really help avoid loading the native resources.

So I am correct in assuming that in situation like this, the only way to prevent the native library from being loaded is to not trigger initialization of the class and subsequent invocation of the static block? We can live with that, it just seems a little fragile.

Or perhaps, would overriding NativeLibraryLocator.getLibraries to return empty list cause no native resources to be loaded and do so with no exception raised? It seems to me the library assumes from its presence that it will be used and that loading native resources in some way is required.

@kwhat
Copy link
Owner

kwhat commented Jan 10, 2019

Hence we tried to isolate the entire library by hiding it behind a facade that loaded lazily on demand, which we had under control (i.e., could refuse to serve on affected platforms).

So there is a bug in Windows that prevents the deletion of the native library on unload. I bring this up because I believe that the solutions to both these problems intersect on a custom class loader for the GlobalScreen. I have no idea how this should/can be implemented, but in theory the code in the class loader will control how GlobalScreen is loaded and you maybe able to figure out a way to work around the issue you found by moving all this static library loading to this class loader. Take a look at https://web.archive.org/web/20140704120535/http://www.codethesis.com/blog/unload-java-jni-dll and https://community.oracle.com/thread/1550086 for some context. If you need help with this approach, I can probably assist.

Or perhaps, would overriding NativeLibraryLocator.getLibraries to return empty list cause no native resources to be loaded and do so with no exception raised?

This approach may work, however, I think you will still run into the java.lang.UnsatisfiedLinkError as soon as GlobalScreen gets loaded by the default class loader. You may try to implement the native methods with a no-op or something to that effect to prevent this error.

@kwhat
Copy link
Owner

kwhat commented Jan 10, 2019

You do know that slf4j is only the adapter, with log4j being one binding to it? In a library you should always take slf4j over log4j.

Yes, I believe that slf4j requires the following at runtime:

<dependency>
    <groupId>org.slf4j</groupId>
    <artifactId>slf4j-api</artifactId>
    <version>1.7.25</version>
</dependency>

If you have a compelling case for slf4j, it can be added for 3.0.

@sghpjuikit
Copy link

sghpjuikit commented Jan 10, 2019

No need for assistance, we have actually written custom class loaders in our project. I'll think about the idea. I think we will end up using the facade that allows us to lazily load the class without class loader magic. Thx for the input.

@xeruf
Copy link
Author

xeruf commented Mar 19, 2019

Alright, I have removed the initialization changes. What about merging it now?

@xeruf
Copy link
Author

xeruf commented May 16, 2019

@kwhat any news?

@xeruf
Copy link
Author

xeruf commented Jun 21, 2019

@kwhat you still alive? ^^

@kwhat
Copy link
Owner

kwhat commented Jun 21, 2019

Unfortunately, yes.

@xeruf
Copy link
Author

xeruf commented Jul 17, 2019

@kwhat can you merge this? I mean, it's only a few small improvements?

@goxr3plus
Copy link

Merge and let's have a dinner.

@deinhofer
Copy link

Hence we tried to isolate the entire library by hiding it behind a facade that loaded lazily on demand, which we had under control (i.e., could refuse to serve on affected platforms).

So there is a bug in Windows that prevents the deletion of the native library on unload. I bring this up because I believe that the solutions to both these problems intersect on a custom class loader for the GlobalScreen. I have no idea how this should/can be implemented, but in theory the code in the class loader will control how GlobalScreen is loaded and you maybe able to figure out a way to work around the issue you found by moving all this static library loading to this class loader. Take a look at https://web.archive.org/web/20140704120535/http://www.codethesis.com/blog/unload-java-jni-dll and https://community.oracle.com/thread/1550086 for some context. If you need help with this approach, I can probably assist.

Or perhaps, would overriding NativeLibraryLocator.getLibraries to return empty list cause no native resources to be loaded and do so with no exception raised?

This approach may work, however, I think you will still run into the java.lang.UnsatisfiedLinkError as soon as GlobalScreen gets loaded by the default class loader. You may try to implement the native methods with a no-op or something to that effect to prevent this error.

I had an issue where it could happen that GloabalScreen.registerNativeHook() could have been called several times which than led to an error.
So I did a manual cleanup of the the extracted .dlls in the temp directory first:

        File tempDir = new File(System.getProperty("java.io.tmpdir"));
        File[] tempDLLs = tempDir.listFiles(new FilenameFilter() {

            @Override
            public boolean accept(File dir, String name) {
                if (name.toLowerCase().startsWith("jnativehook") && name.endsWith(".dll")) {
                    return true;
                }

                return false;
            }
        });
        for (File tempDLL : tempDLLs) {
            tempDLL.delete();
        }

@kwhat kwhat changed the base branch from 2.1 to 2.2 August 26, 2020 00:36
@kwhat kwhat self-assigned this Aug 26, 2020
@kwhat kwhat added this to the 2.2.0 milestone Aug 26, 2020
@kwhat kwhat removed this from the 2.2.0 milestone Sep 27, 2021
@kwhat kwhat changed the base branch from 2.2_ to 2.2 October 13, 2021 19:04
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.

Getting mouse debug inside eclipse console Do not load statically
5 participants