glassfish
  1. glassfish
  2. GLASSFISH-15769

Regression: Weaving attempted in Glassfish embedded even though supposedly disabled

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 3.1_b39
    • Fix Version/s: 3.1_ms08
    • Component/s: entity-persistence
    • Labels:
      None

      Description

      Weaving is apparently disabled-or at least that's the intent-in Glassfish embedded.

      However, when I use the embeddable Glassfish APIs, documented at http://embedded-glassfish.java.net/nonav/apidocs/, it appears that weaving is attempted.

      I have an EJB project set up with a persistence unit in it. So, some classes are EJB interfaces, others are EJB implementations, and others are entity classes. There is a META-INF/persistence.xml file in there. javax.ejb.embeddable.EJBContainer deploys this "archive" fine.

      I've recently switched over to using the Glassfish embeddable APIs instead, since they are more robust (they support remote EJBs among other things). I've made no other changes.

      At test time, I get the exception message detailed by this bug: https://bugs.eclipse.org/bugs/show_bug.cgi?id=323403

      That is, EclipseLink 2.2.0 is attempting to invoke a weaving-supplied method, even though weaving is disabled.

      I suspect that something about the way weaving is "disabled" is not in fact fully disabling it.

      In point of fact, I would like weaving to be ON. But I understand that this is an issue with an embedded API.

      I can work around this issue if I explicitly set weaving to be off in my persistence.xml file:

      <property name="eclipselink.weaving" value="false"/>

      ...but I'd rather not do that.

        Activity

        ljnelson created issue -
        Bhavanishankar made changes -
        Field Original Value New Value
        Assignee Bhavanishankar [ bhavanishankar ] Mitesh Meswani [ mm110999 ]
        Mitesh Meswani made changes -
        Tags 3_1-fishcat 3_1-fishcat 3_1-review
        Hide
        Mitesh Meswani added a comment -

        Weaving was always disabled in embedded mode to guard against use cases where entity classes are touched (and hence loaded) by the app before transformers get installed. JPADeployer relied on serverEnvironment.isEmbedded() to determine if it is being run in embedded mode. Embedded implementation has recently changed to not answer true for serverEnvironment.isEmbedded() causing the regression. After discussions, it seems instead of always disabling weaving, a more flexible approach would be to enabled it by default and give user an option to explicitly disable it for embedded. Following diffs implements the change

        Index: src/main/java/org/glassfish/persistence/jpa/JPADeployer.java
        ===================================================================
        — src/main/java/org/glassfish/persistence/jpa/JPADeployer.java (revision 44872)
        +++ src/main/java/org/glassfish/persistence/jpa/JPADeployer.java (working copy)
        @@ -42,6 +42,7 @@

        import com.sun.appserv.connectors.internal.api.ConnectorRuntime;
        import com.sun.enterprise.deployment.*;
        +import com.sun.enterprise.module.bootstrap.StartupContext;
        import com.sun.logging.LogDomains;
        import org.glassfish.api.deployment.DeployCommandParameters;
        import org.glassfish.api.event.EventListener;
        @@ -84,6 +85,9 @@
        private ServerEnvironmentImpl serverEnvironment;

        @Inject
        + private volatile StartupContext sc = null;
        +
        + @Inject
        private Events events;

        @Inject
        @@ -196,7 +200,13 @@
        @Override void visitPUD(PersistenceUnitDescriptor pud, DeploymentContext cont
        ext) {
        if(referencedPus.contains(pud)) {
        boolean isDas = isDas();

        • ProviderContainerContractInfo providerContainerContractInfo = serverEnvironment.isEmbedded() ?
          +
          + // While running in embedded mode, it is not possible to guarantee that entity classes are not loaded by the app classloader before transformers are installed
          + // If that happens, weaving will not take place and EclipseLink will throw up. Provide users an option to disable weaving by passing the flag.
          + // Note that we enable weaving if not explicitly disabled by user
          + boolean weavingEnabled = Boolean.valueOf(sc.getArguments().getProperty("org.glassfish.persistence.embedded.weaving.enabled", "true"));
          +
          + ProviderContainerContractInfo providerContainerContractInfo = weavingEnabled ?
          new EmbeddedProviderContainerContractInfo(context, connectorRuntime, isDas) :
          new ServerProviderContainerContractInfo(context, connectorRuntime, isDas);
          PersistenceUnitLoader puLoader = new PersistenceUnitLoader(pud, providerContainerContractInfo);
        Show
        Mitesh Meswani added a comment - Weaving was always disabled in embedded mode to guard against use cases where entity classes are touched (and hence loaded) by the app before transformers get installed. JPADeployer relied on serverEnvironment.isEmbedded() to determine if it is being run in embedded mode. Embedded implementation has recently changed to not answer true for serverEnvironment.isEmbedded() causing the regression. After discussions, it seems instead of always disabling weaving, a more flexible approach would be to enabled it by default and give user an option to explicitly disable it for embedded. Following diffs implements the change Index: src/main/java/org/glassfish/persistence/jpa/JPADeployer.java =================================================================== — src/main/java/org/glassfish/persistence/jpa/JPADeployer.java (revision 44872) +++ src/main/java/org/glassfish/persistence/jpa/JPADeployer.java (working copy) @@ -42,6 +42,7 @@ import com.sun.appserv.connectors.internal.api.ConnectorRuntime; import com.sun.enterprise.deployment.*; +import com.sun.enterprise.module.bootstrap.StartupContext; import com.sun.logging.LogDomains; import org.glassfish.api.deployment.DeployCommandParameters; import org.glassfish.api.event.EventListener; @@ -84,6 +85,9 @@ private ServerEnvironmentImpl serverEnvironment; @Inject + private volatile StartupContext sc = null; + + @Inject private Events events; @Inject @@ -196,7 +200,13 @@ @Override void visitPUD(PersistenceUnitDescriptor pud, DeploymentContext cont ext) { if(referencedPus.contains(pud)) { boolean isDas = isDas(); ProviderContainerContractInfo providerContainerContractInfo = serverEnvironment.isEmbedded() ? + + // While running in embedded mode, it is not possible to guarantee that entity classes are not loaded by the app classloader before transformers are installed + // If that happens, weaving will not take place and EclipseLink will throw up. Provide users an option to disable weaving by passing the flag. + // Note that we enable weaving if not explicitly disabled by user + boolean weavingEnabled = Boolean.valueOf(sc.getArguments().getProperty("org.glassfish.persistence.embedded.weaving.enabled", "true")); + + ProviderContainerContractInfo providerContainerContractInfo = weavingEnabled ? new EmbeddedProviderContainerContractInfo(context, connectorRuntime, isDas) : new ServerProviderContainerContractInfo(context, connectorRuntime, isDas); PersistenceUnitLoader puLoader = new PersistenceUnitLoader(pud, providerContainerContractInfo);
        Hide
        Mitesh Meswani added a comment -

        How bad is its impact? (Severity)
        Identify why the fix needs to occur now:

        • Is a regression of functionality from prior release.
        • With this issue present, if an app needs to disable weaving for embedded, the app's persistence.xml needs to be changed. This is quite inconvenient for unit test scenarios (The main use case for embedded)

        How often does it happen? (Frequency)

        • For any app that needs to disable weaving

        How much effort is required to fix it? (Cost)

        • The fix is available and is attached in previous comment

        What is the risk of fixing it? (Risk)

        • Minimal to no risk. The fix is localized to embedded use case. I have verified in debugger that nothing is changed for non embedded case.

        Does a work around for the issue exist? Can the workaround be reasonably employed by the end user?

        • Work around is to modify persistence.xml to explicitly disable weaving. This makes using embedded for unit testing inconvenient

        If the issue is not fixed should the issue and its workaround (if applicable) be described in the Release Notes?

        • Yes

        How long has the bug existed in the product?

        • Not more than a month

        Do regression tests exist for this issue?

        • No.

        Which tests should QA (re)run to verify the fix did not destabilize GlassFish?

        • The standard QE test suit against derby. I have already run them with these changes.

        When will a tested fix be ready for integration?

        • It is available now.
        Show
        Mitesh Meswani added a comment - How bad is its impact? (Severity) Identify why the fix needs to occur now: Is a regression of functionality from prior release. With this issue present, if an app needs to disable weaving for embedded, the app's persistence.xml needs to be changed. This is quite inconvenient for unit test scenarios (The main use case for embedded) How often does it happen? (Frequency) For any app that needs to disable weaving How much effort is required to fix it? (Cost) The fix is available and is attached in previous comment What is the risk of fixing it? (Risk) Minimal to no risk. The fix is localized to embedded use case. I have verified in debugger that nothing is changed for non embedded case. Does a work around for the issue exist? Can the workaround be reasonably employed by the end user? Work around is to modify persistence.xml to explicitly disable weaving. This makes using embedded for unit testing inconvenient If the issue is not fixed should the issue and its workaround (if applicable) be described in the Release Notes? Yes How long has the bug existed in the product? Not more than a month Do regression tests exist for this issue? No. Which tests should QA (re)run to verify the fix did not destabilize GlassFish? The standard QE test suit against derby. I have already run them with these changes. When will a tested fix be ready for integration? It is available now.
        Nazrul made changes -
        Tags 3_1-fishcat 3_1-review 3_1-approved 3_1-fishcat 3_1-review
        Nazrul made changes -
        Tags 3_1-approved 3_1-fishcat 3_1-review 3_1-approved 3_1-fishcat
        Hide
        Mitesh Meswani added a comment - - edited

        Checked in the code branch rev 44890, trunk rev 44889

        Show
        Mitesh Meswani added a comment - - edited Checked in the code branch rev 44890, trunk rev 44889
        Hide
        Mitesh Meswani added a comment -

        Assigning to docs to document the property "org.glassfish.persistence.embedded.weaving.enabled" in Embedded guide

        Show
        Mitesh Meswani added a comment - Assigning to docs to document the property "org.glassfish.persistence.embedded.weaving.enabled" in Embedded guide
        Mitesh Meswani made changes -
        Assignee Mitesh Meswani [ mm110999 ] Paul Davies [ pauldavies ]
        Component/s docs [ 10597 ]
        Component/s embedded [ 10643 ]
        Paul Davies made changes -
        Summary Weaving attempted in Glassfish embedded even though supposedly disabled [UB]Weaving attempted in Glassfish embedded even though supposedly disabled
        Assignee Paul Davies [ pauldavies ] Rebecca Parks [ junesm ]
        Fix Version/s 3.1_ms08 [ 11017 ]
        Hide
        Sanjeeb Sahoo added a comment -

        I am not convinced by the change. There seem to be an assumption made about embedded which is not true. Why can't weaving be disabled outside embedded mode? Similarly, what happens when weaving has not disabled in embedded mode using this system property, but it has been explicitly disabled using persistence.xml? Are we loosing out any functionality by not using EmbeddedProviderContractInfo? Or, is it just a matter of name here?

        Show
        Sanjeeb Sahoo added a comment - I am not convinced by the change. There seem to be an assumption made about embedded which is not true. Why can't weaving be disabled outside embedded mode? Similarly, what happens when weaving has not disabled in embedded mode using this system property, but it has been explicitly disabled using persistence.xml? Are we loosing out any functionality by not using EmbeddedProviderContractInfo? Or, is it just a matter of name here?
        Hide
        Mitesh Meswani added a comment -

        What do you mean by weaving disabled outside embedded mode?

        > what happens when weaving has not disabled in embedded mode using this system property, but it has been explicitly disabled using persistence.xml?
        It will be disabled.

        >Are we loosing out any functionality by not using EmbeddedProviderContractInfo? Or, is it just a matter of name here?
        No. It is just matter of name. EmbeddedProviderContractInfo currently just provides isWeavingDisabled(). Once the trunk opens up, I am inclined towards removing the class with some refactoring.

        Show
        Mitesh Meswani added a comment - What do you mean by weaving disabled outside embedded mode? > what happens when weaving has not disabled in embedded mode using this system property, but it has been explicitly disabled using persistence.xml? It will be disabled. >Are we loosing out any functionality by not using EmbeddedProviderContractInfo? Or, is it just a matter of name here? No. It is just matter of name. EmbeddedProviderContractInfo currently just provides isWeavingDisabled(). Once the trunk opens up, I am inclined towards removing the class with some refactoring.
        Hide
        marina vatkina added a comment -

        I'm confused... ServerEnvironmentImpl.isEmbedded() returns true in static-shell run of the EJB Timer Service...

        Show
        marina vatkina added a comment - I'm confused... ServerEnvironmentImpl.isEmbedded() returns true in static-shell run of the EJB Timer Service...
        Hide
        marina vatkina added a comment -

        All JPA tests in embedded ejb devtests failed - see http://hudson-sca.us.oracle.com/job/ejb-devtests-v3/281/

        Show
        marina vatkina added a comment - All JPA tests in embedded ejb devtests failed - see http://hudson-sca.us.oracle.com/job/ejb-devtests-v3/281/
        marina vatkina made changes -
        Assignee Rebecca Parks [ junesm ] Mitesh Meswani [ mm110999 ]
        Priority Major [ 3 ] Critical [ 2 ]
        Component/s entity-persistence [ 10624 ]
        Component/s docs [ 10597 ]
        marina vatkina made changes -
        Summary [UB]Weaving attempted in Glassfish embedded even though supposedly disabled Regression: Weaving attempted in Glassfish embedded even though supposedly disabled
        Hide
        Mitesh Meswani added a comment -

        Checked in 44918 in trunk and 44919 in branch.
        The check in disables weaving for embedded EJB container case as was before. The status now is:
        -Weaving is enabled by default when GlassFish Emedded API is used. It can be disabled by specifying property "org.glassfish.persistence.embedded.weaving.enabled" as "false"
        -Weaving is disabled for embedded EJB container use case.

        This behavior is not yet stable and can be changed in future versions of GlassFish.

        Show
        Mitesh Meswani added a comment - Checked in 44918 in trunk and 44919 in branch. The check in disables weaving for embedded EJB container case as was before. The status now is: -Weaving is enabled by default when GlassFish Emedded API is used. It can be disabled by specifying property "org.glassfish.persistence.embedded.weaving.enabled" as "false" -Weaving is disabled for embedded EJB container use case. This behavior is not yet stable and can be changed in future versions of GlassFish.
        Mitesh Meswani made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        ljnelson added a comment -

        Thanks for your hard work.

        Show
        ljnelson added a comment - Thanks for your hard work.
        Hide
        Sanjeeb Sahoo added a comment -

        Replying to Mitesh's response:

        > What do you mean by weaving disabled outside embedded mode?
        What this means is for whatever reason, someone may like to disable weaving in regular glassfish - e.g., someone wants to measure benefits of weaving, but they don't want to change their persistence.xmls to set the property. In such a case, the new system property can be very handy.

        >> what happens when weaving has not disabled in embedded mode using this system property, but it has been explicitly disabled using persistence.xml?
        > It will be disabled.
        That's the desired behavior.

        >> Are we loosing out any functionality by not using EmbeddedProviderContractInfo? Or, is it just a matter of name here?
        > No. It is just matter of name. EmbeddedProviderContractInfo currently just provides isWeavingDisabled(). Once the trunk opens up, > I am inclined towards removing the class with some refactoring.

        I look forward to this improvement to improve clarity in the code.

        Thanks.

        Show
        Sanjeeb Sahoo added a comment - Replying to Mitesh's response: > What do you mean by weaving disabled outside embedded mode? What this means is for whatever reason, someone may like to disable weaving in regular glassfish - e.g., someone wants to measure benefits of weaving, but they don't want to change their persistence.xmls to set the property. In such a case, the new system property can be very handy. >> what happens when weaving has not disabled in embedded mode using this system property, but it has been explicitly disabled using persistence.xml? > It will be disabled. That's the desired behavior. >> Are we loosing out any functionality by not using EmbeddedProviderContractInfo? Or, is it just a matter of name here? > No. It is just matter of name. EmbeddedProviderContractInfo currently just provides isWeavingDisabled(). Once the trunk opens up, > I am inclined towards removing the class with some refactoring. I look forward to this improvement to improve clarity in the code. Thanks.
        Hide
        ljnelson added a comment -
        Show
        ljnelson added a comment - Please also see http://java.net/jira/browse/GLASSFISH-13688
        ljnelson made changes -
        Comment [ I should mention that (hopefully obviously) when I disable weaving from my persistence.xml everything works OK.

        It also appears to me that weaving is NOT enabled in the absence of the system property. That is, I can't make the "_vh_ method" exception above disappear by doing anything other than disabling weaving from my persistence.xml file. ]
        ljnelson made changes -
        Comment [ I have set the system property mentioned above to false.

        As I understand it, this should prevent EclipseLink from attempting to weave anything.

        I am using the embedded Glassfish APIs directly--build 41.

        I get the following error from my unit test:

        Exception [EclipseLink-218] (Eclipse Persistence Services - 2.2.0.v20110202-r8913): org.eclipse.persistence.exceptions.DescriptorException
        Exception Description: A NullPointerException would have occurred accessing a non-existent weaved _vh_ method [_persistence_get_person_vh]. The class was not weaved properly - for EE deployments, check the module order in the application.xml deployment descriptor and verify that the module containing the persistence unit is ahead of any other module that uses it.

        There is no mention in my persistence.xml file of any weaving property at all.

        Have I misunderstood something? ]

          People

          • Assignee:
            Mitesh Meswani
            Reporter:
            ljnelson
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: