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

BOX-127 create a simple service for making portable in Lucee #5

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

Conversation

DominicWatson
Copy link
Contributor

Create a dedicated java loading proxy service for getting at the java objects from the RabbitMQ library. This allows some simple logic to behave differently when Lucee than with ACF. i.e. we can just use CreateObject( "java", classPath, libJars ) to dynamically load the jars.

We could possibly use this same approach to use JavaLoader with ACF if there are no problems with JL + ACF?

@bdw429s bdw429s changed the base branch from master to development June 29, 2022 18:05
@bdw429s
Copy link
Collaborator

bdw429s commented Jun 29, 2022

This is an interesting idea. Here is the Lucee ticket that drove me away from using JavaLoader on this specific project:
https://luceeserver.atlassian.net/browse/LDEV-2296
It's 3 years old and hasn't had any traffic from Micha yet despite me narrowing down the exact point in the Lucee code things break down.

And on a related note, here's the 7 year old ticket for Adobe to finally catch up with Lucee and provide the same mechanism for providing jars on the fly to createObject().
https://tracker.adobe.com/#/view/CF-4087803

Have you tested this change on both Lucee and Adobe? I'm also curious if it causes any issues to have the Rabbit jars in the this.JavaSettings or the servlet's lib folder and ALSO use the new approach? If so, this would be a breaking change since the docs have previously instructed all users to manually supply the jars.

I'm not against using JavaLoader for Adobe, but I'd REALLY like to avoid it on my projects. It's very, very old and basically has zero support. It can also easily cause memory leaks if it's not shutdown properly, which is even harder to enforce when outside of the ColdBox/CommandBox frameworks.

@DominicWatson
Copy link
Contributor Author

Yeah agreed. We don't use javaloader anywhere. I think this PR needs more work - the RabbitMQ java library is built with OSGi meta so could be refactored further by tapping into that. I'm actively working on this right now so I'll try to get OSGi working.

I haven't run the tests using ACF, was presuming without looking that there would be an engine matrix for running the tests in Gitlab - but I wouldn't foresee any issue here and will verify. But if not wanting JavaLoader, then for ACF, this change shouldn't effect it in any way - just add a smoother experience for Lucee users. Having it configurable and off by default could be an option though? i.e. if on ACF, and not able/wanting to update lib dirs, then could switch it on with some config?

Begs the question, does this warrant a separate mini library for loading jars that uses Lucee native where possible (and using OSGi where possible), and falling back to ACF compatible options otherwise??

@bdw429s
Copy link
Collaborator

bdw429s commented Jun 30, 2022

the RabbitMQ java library is built with OSGi meta so could be refactored further by tapping into that. I'm actively working on this right now so I'll try to get OSGi working.

Meh, usually OSGI means everything gets more complicated, so I don't personally care much about that unless you just really want to, lol 😄

was presuming without looking that there would be an engine matrix for running the tests in Gitlab

I'm using Github Actions for this, but sadly the build needs some love and I only have the tests running on Lucee ATM. Feel free to help add Adobe into the mix. The client who paid for most of this work only cared about Lucee so my Adobe support was just some local testing I did.

then for ACF, this change shouldn't effect it in any way

Agreed, but we should at least just hit the tests to ensure the code paths are ok. For example, will Adobe freak out when it goes to compile a createObject() call with too many args, etc.

Having it configurable

This isn't a bad idea just in case someone has a really good reason to load the jars themselves.

off by default

Honestly, so long as it works, I'm fine with it on by default as I really prefer it to "just work". We can do a major bump of the library version just in case since it could be a breaking change for someone somewhere. (not many people are using this from what I can tell)

Begs the question, does this warrant a separate mini library for loading jars that uses Lucee native where possible (and using OSGi where possible), and falling back to ACF compatible options otherwise??

You know, I thought about the same thing, lol. I really hate having two installation methods for different engines, and the annoying thing is just having cbjavaloader in the box.json as a dependency will mean it gets downloaded and loaded even if we hit a code path that ignores it. Most of my other libraries which wrap jars still use javaloader like bcrypt, cfcouchbase, etc. So long as those continue to work as-is I'm not of the mind to change them. What I'd REALLY like is for Adobe to get their butts in gear and provide me with proper cross-engine support so I don't have to keep wasting my time to make up for their lack of progress. So the future I see is one where no shim library is required which makes me reticent to move that direction even though there is no indication that the current status quo will change any time soon 😞

On a related note, it would be interesting if we just modified our port of javaloader to detect Lucee and perform your workaround instead of creating the custom classloaders, etc. It's not any less work or annoyance, but it does still keep us just maintaining a single module for cross-engine compat java class loading.

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.

2 participants