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

Stack overflow scheduling new ManagedScheduledExecutor task from task #6527

Closed
rowland66 opened this issue Feb 14, 2019 · 5 comments · Fixed by #6846
Closed

Stack overflow scheduling new ManagedScheduledExecutor task from task #6527

rowland66 opened this issue Feb 14, 2019 · 5 comments · Fixed by #6846
Assignees

Comments

@rowland66
Copy link

I have an application the implements a pattern of using the JEE ManagedScheduledExecutor to run background tasks. When a task runs, it may schedule itself to run again either immediately, or with some delay. On WAS 18.0,0.3, this process leads to a stack overflow with the following stack trace (shortened for readability):

  at com.ibm.ws.javaee.metadata.context.ejb.EJBComponentMetaDataWrapper.getJ2EEName(EJBComponentMetaDataWrapper.java:60)
  at com.ibm.ws.javaee.metadata.context.ejb.EJBComponentMetaDataWrapper.getJ2EEName(EJBComponentMetaDataWrapper.java:60)
  at com.ibm.ws.javaee.metadata.context.ejb.EJBComponentMetaDataWrapper.getJ2EEName(EJBComponentMetaDataWrapper.java:60)
  at com.ibm.ws.javaee.metadata.context.ejb.EJBComponentMetaDataWrapper.getJ2EEName(EJBComponentMetaDataWrapper.java:60)
  at com.ibm.ws.javaee.metadata.context.ejb.EJBComponentMetaDataWrapper.getJ2EEName(EJBComponentMetaDataWrapper.java:60)
  at com.ibm.ws.javaee.metadata.context.ejb.EJBComponentMetaDataWrapper.getJ2EEName(EJBComponentMetaDataWrapper.java:60)
  at com.ibm.ws.javaee.metadata.context.ejb.EJBComponentMetaDataWrapper.getJ2EEName(EJBComponentMetaDataWrapper.java:60)
  at com.ibm.ws.javaee.metadata.context.ejb.EJBComponentMetaDataWrapper.getJ2EEName(EJBComponentMetaDataWrapper.java:60)
  at com.ibm.ws.javaee.metadata.context.ejb.EJBComponentMetaDataWrapper.getJ2EEName(EJBComponentMetaDataWrapper.java:60)
  at com.ibm.ws.javaee.metadata.context.ejb.EJBComponentMetaDataWrapper.getPersistentIdentifier(EJBComponentMetaDataWrapper.java:92)
  at com.ibm.ws.container.service.metadata.internal.MetaDataServiceImpl.getMetaDataIdentifier(MetaDataServiceImpl.java:196)
  at com.ibm.ws.context.service.serializable.ThreadContextDescriptorImpl.<init>(ThreadContextDescriptorImpl.java:197)
  at com.ibm.ws.context.service.serializable.ThreadContextManager.captureThreadContext(ThreadContextManager.java:117)
  at com.ibm.ws.concurrent.internal.ContextServiceImpl.captureThreadContext(ContextServiceImpl.java:260)
  at com.ibm.ws.concurrent.internal.ContextServiceImpl.captureThreadContext(ContextServiceImpl.java:207)
  at com.ibm.ws.concurrent.internal.ScheduledTask.<init>(ScheduledTask.java:217)
  at com.ibm.ws.concurrent.internal.ManagedScheduledExecutorServiceImpl.schedule(ManagedScheduledExecutorServiceImpl.java:155)
  at com.dovetailsys.q5.gateway.ejb.GatewayControllerOperationsBean$GatewayTask.run(GatewayControllerOperationsBean.java:1299)
  at com.ibm.ws.concurrent.internal.ScheduledTask.call(ScheduledTask.java:383)
  at java.util.concurrent.FutureTask.run(FutureTask.java:266)
  at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
  at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
  at java.lang.Thread.run(Thread.java:748)

It appears that when a task is scheduled the current thread context is captured and in that process EJBComponentMetaData is wrapped in a wrapper class EJBComponentMetaDataWrapper. This process repeats every time a task is scheduled creating one more wrapper around the existing wrapper. Eventually a call to a wrapper method triggers a stack overflow.

I have fixed this with a simple change to the com.ibm.ws.javaee.metadata.context.ejb.EJBComponentMetaDataWrapper constructor. The altered constructor checks whether the EJBComponentMetaData passed to it is an EJBComponentMetaDataWrapper. If it is, it wraps the underlying EJBMetaDataComponentData object and not the wrapper. This change eliminated the errors that I was seeing.

New EJBComponentMetaDataWrapper constructor:

public EJBComponentMetaDataWrapper(EJBComponentMetaData metadata) {
    if (metadata instanceof EJBComponentMetaDataWrapper) {
		this.ejbComponentMetaData = ((EJBComponentMetaDataWrapper) metadata).ejbComponentMetaData;
	} else {
		this.ejbComponentMetaData = metadata;
	}
}

As I am not familiar with how this process of saving the thread context works, and how many other wrapper classes might exist, I am not sure if this fix is the only fix that is needed. It was enough to fix my issue.

@aguibert
Copy link
Contributor

aguibert commented Feb 14, 2019

hi @rowland66, thanks for reporting this issue and digging into it!

I think the proposed code change would avoid the stack overflow, but it would still result in a lot of extra EJBComponentMetaDataWrapper instances being created and potentially run out of memory eventually.

Instead, I think we could make a change in com.ibm.ws.javaee.metadata.context.ejb.EJBComponentMetaDataDecoratorImpl to not wrap an existing wrapper like this:

    @Override
    public ComponentMetaData decorate(ComponentMetaData metadata) {
        if (metadata instanceof EJBComponentMetaDataWrapper)
            return metadata;
        else if (metadata instanceof EJBComponentMetaData)
            return new EJBComponentMetaDataWrapper((EJBComponentMetaData) metadata);
        else
            return metadata;
    }

@aguibert aguibert added in:EJB Container bug This bug is not present in a released version of Open Liberty labels Feb 14, 2019
@rowland66
Copy link
Author

I agree that my fix may not be particularly efficient. As I don't really understand the purpose of the EJBComponentMetaDataWrapper I can't say whether sharing the wrapper between scheduled tasks would be a problem. It looks like the wrapper only changes the valued returned by the getPersistentIdentifier() method, but the returned value would be the same for all wrappers. So your approach should work. If you would like to create a patch for me to test, I would be glad to do that.

@aguibert
Copy link
Contributor

since it looks like you already went through the steps of creating and testing your initial patch, would you like to just build and test the patch I proposed and then make a PR if it works? Since you did most of the work investigating and testing this issue, I'd be happy to let you receive credit for contributing the fix. The only extra step required would be signing the contributor agreement.

If you're not interested in contributing the fix, then I can certainly take it forward from here and produce a patch for you.

@aguibert aguibert added release bug This bug is present in a released version of Open Liberty and removed bug This bug is not present in a released version of Open Liberty labels Feb 14, 2019
@tkburroughs
Copy link
Member

BTW... the purpose of the EJBComponentMetaDataWrapper is to trick the EJBUserTransactionDecorator into allowing the lookup of java:comp/UserTransaction from the thread. java:comp/UserTransaction is normally not available for Container Managed Transaction EJBs; however this work is not technically running an EJB method, only in the context of that EJB and should always have access to java:comp/UserTransaction.

There is definitely no need to have wrappered wrappers. The update to decorate looks great.

@rowland66
Copy link
Author

Hi Andrew. I am happy to forgo recognition for this fix. I have tested your changes with the 18.0.0.3 release and it seems to work as well as mine did. Please go ahead and make the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants