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

EJBCLIENT-241 Shut down executor service in EJBClientContext #283

Closed
wants to merge 1 commit into from

Conversation

TomasHofman
Copy link
Contributor

@elguardian
Copy link
Contributor

Aren't you getting rejection execution exception with this change ?

@TomasHofman
Copy link
Contributor Author

So far I didn't notice, I'll do some more experiments. Are you getting those or just suspecting?

@elguardian
Copy link
Contributor

I just suspect that could happen... but very edge cases. You should ask David. I remember that the shutdown was there at some point but cannot remember when.

@TomasHofman
Copy link
Contributor Author

I checked the code once more - tasks are submitted to ejbClientContextTasksExecutorService in methods:

  • registerEJBReceiver()
  • unregisterEJBReceiver()
  • attemptReconnections()

Intuitively I think none of these methods should be called on closed context, apart perhaps from unregisterEJBReceiver(). All ejb receivers are however unregistered in the close() method, so there's no reason to do that again (but who knows).

https://github.com/jbossas/jboss-eap7/blob/7.0.x/ejb3/src/main/java/org/jboss/as/ejb3/remote/DescriptorBasedEJBClientContextService.java#L117 - here it calls unregister and close in correct orther.

@elguardian
Copy link
Contributor

attemptReconnections can be executed at any time... that is the reason you have a close check in the ReconnectHandler... The offending commit about not shuting down is this one:

87aef56

@bmaxwell can give away more info about why he removed that line

@TomasHofman
Copy link
Contributor Author

Hmm, that looks quite relevant.

@elguardian
Copy link
Contributor

elguardian commented Jun 20, 2017

I think you can bypass the problem related to execute tasks avoiding the rejected execution exception. The trick would be to use the RejectedExecutionHandler somethink like this....

ThreadPoolExecutor service = (ThreadPoolExecutor) Executors.newCachedThreadPool(new DaemonThreadFactory("ejb-client-context-tasks"));
service.setRejectedExecutionHandler(new RejectedExecutionHandler() {

	@Override
	public void rejectedExecution(Runnable r, ThreadPoolExecutor executor) {
		if(executor.isShutdown()) {
			System.out.println("this in theory should not happen");
		}
		r.run();
	}
			
});

so even if you shutdown the executor you will execute the task in the caller thread... You can add some stacktrace if you are calling the executor after closing the context.

@TomasHofman
Copy link
Contributor Author

The thing is I didn't manage to reproduce RejectedExecutionHandler at all. It may've been a problem in earlier version which is now fixed.

@TomasHofman
Copy link
Contributor Author

Closing this for now until the other necessary fixes are figured out.

@TomasHofman TomasHofman closed this Aug 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants