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

Jakarta EE 9: Enterprise Beans 4.0 - reduce need for PortableRemoteObject.narrow() #16238

Closed
tkburroughs opened this issue Mar 15, 2021 · 2 comments
Assignees
Labels
Epic Used to track Feature Epics that are following the UFO process in:EJB Container In Progress Items that are in active development. jakartaEE9 story team:Blizzard

Comments

@tkburroughs
Copy link
Member

tkburroughs commented Mar 15, 2021

The EJB 2.x APIs currently require the use of PortableRemoteObject.narrow() after performing a JNDI lookup of the EJBHome remote interface. As long as the specific EJBHome interface is available on the client classpath, the use of PortableRemoteObject.narrow() should not be required by the Liberty implementation. To maintain application portability, an EJB 2.x API (i.e. Homes) application should still include the call.

In addition, several of the EJB FAT are unnecessarily performing a PRO.narrow() for EJB 3.x API interfaces. These should be removed to ensure the FAT is testing compliance with the specification. Pull request #16272 is an initial removal of the obvious unnecessary use of PRO.narrow, but there are likely others which are not as obvious because they are using utility methods to perform the lookups.

@tkburroughs
Copy link
Member Author

tkburroughs commented Mar 17, 2021

I did a proof of concept for this as follows:

Change the return in com.ibm.ws.ejbcontainer.remote.internal.EJBServantLocatorImpl.createReference() from this:

        return adapter.create_reference_with_id(oid, CORBA_Utils.getRemoteTypeId(intf));

to this

        Object stub = adapter.create_reference_with_id(oid, CORBA_Utils.getRemoteTypeId(intf));
        System.out.println("EJBServantLocatorImpl.createReference:stub=" + stub.getClass().getName());
        EJSContainer.getThreadData().pushClassLoader(w.bmd);
        stub = PortableRemoteObject.narrow(stub, intf);
        System.out.println("EJBServantLocatorImpl.createReference:stub=" + stub.getClass().getName());
        new Exception().printStackTrace(System.out);
        EJSContainer.getThreadData().popClassLoader();
        return stub;

This allowed many FAT that perform JNDI lookups of EJBHome interfaces to work without performing a PRO.narrow().
However, this is not the correct approach, nor will it always work for the following reasons:

1 - EJBServantLocatorImpl.createReference() will be called for basically every JNDI lookup, so the EJB 3.x API paths will already be performing a PRO.narrow()..... thus the PRO.narrow() call would occur twice. EJBServantLocatorImpl.createReference() was very specifically designed to not perform the narrow; it should be added elsewhere.
2 - The POC is performing a narrow to the home interface class loaded by the app classloader of the application that contains the EJB.... thus, this POC will only work if the JNDI lookup is performed by the same application as where the EJB is defined. The cast will fail if the lookup is performed across two different applications.
3 - The call to pushClassLoader(w.bmd) should not be needed. It had to be added for the POC to avoid a Java 2 Security permission problem, which should not be necessary if the home interface class being narrowed to was loaded from the client app classloader rather than the EJB classloader; or at least this is the likely cause.

Note: If you remove the calls to pushClassLoader(w.bmd) and popClassLoader() from the POC then this will result in Java 2 Security violations, which is another way to show the code paths (stack dump) of the various ways get to obtain an EJB reference.

To find a more appropriate place to make this enhancement, I recommend looking at com.ibm.ws.ejbcontainer.osgi.internal.naming.EJBNamingInstancer.initializeEJB(); specifically at this code:

            EJSHome home = binding.homeRecord.getHomeAndInitialize();

            if (binding.isHome()) {
                EJSWrapperCommon wc = home.getWrapper();
                if (binding.isLocal) {
                    instance = wc.getLocalObject();
                } else {
                    instance = home.getContainer().getEJBRuntime().getRemoteReference(wc.getRemoteWrapper());
                }
            } else {
                // Use interface name to create the business object
                if (binding.isLocal) {
                    instance = home.createLocalBusinessObject(binding.interfaceIndex, null);
                } else {
                    instance = home.createRemoteBusinessObject(binding.interfaceIndex, null);
                }
            }

The EJB 3.x API interfaces will call home.createRemoteBusinessObject(), which will end up performing the narrow. I'd recommend adding a similar method to EJSHome for the EJB 2.x home interfaces, which would perform the narrow for home interfaces. The actual narrow call would perhaps be best on a new method of EJSWrapperCommon, similar to getRemoteBusinessReference, which is where the narrow is performed for EJB 3.x. Note that EJSWrapperCommon.getRemoteBusinessRefernce appears to have the proper mechanism to narrow to the interface loaded by the client classloader.

tkburroughs added a commit to tkburroughs/open-liberty that referenced this issue Mar 17, 2021
Some FAT for the EJB 3.x APIs are unnecessarily performing PortableRemoteObject.narrow()
calls; remove those calls to ensure compliance with the specification.
tkburroughs added a commit that referenced this issue Mar 18, 2021
@jhanders34 jhanders34 added the Epic Used to track Feature Epics that are following the UFO process label Apr 27, 2021
meiaus pushed a commit to meiaus/open-liberty that referenced this issue May 18, 2021
Some FAT for the EJB 3.x APIs are unnecessarily performing PortableRemoteObject.narrow()
calls; remove those calls to ensure compliance with the specification.
@tkburroughs tkburroughs added the In Progress Items that are in active development. label Jun 9, 2021
@olendvcook
Copy link
Contributor

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Epic Used to track Feature Epics that are following the UFO process in:EJB Container In Progress Items that are in active development. jakartaEE9 story team:Blizzard
Projects
None yet
Development

No branches or pull requests

3 participants