-
Notifications
You must be signed in to change notification settings - Fork 31
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
Added wildfly12 profile #165
Conversation
Failed test cases ( |
@Override | ||
public AsyncFuture<ModelNode> executeAsync(ModelNode modelNode, OperationMessageHandler handler) { | ||
throw new UnsupportedOperationException("Asynchronous execution is not supported by " + getClass().getName()); | ||
ExecutorService executor = Executors.newSingleThreadExecutor(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is the executor shut down? I guess the ...AsyncFutureTask
doesn't shutdown the executor after it's completed, so you'll keep leaking threads.
Given that this class has a close
method, maybe it would be better to have one Executors.newCachedThreadPool()
executor for the entire instance of this class, which you could shutdown in the close
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for advice. Calling shutdown()
was missing. Use only one ExecutorService
for entire instance is better solution. I fixed it in suggested way.
} | ||
} | ||
|
||
private ExecutorService obtainExecutorService() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the executor service needs to be created lazily; if the client is created, it's extremely likely to be used :-) But I'll leave it to you -- this is good too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created it lazily because older version than WildFly 12 still uses execute
method instead of executeAsync
. I believe it is better to create ExecutorService only on demand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, very much true! Makes sense.
Added wildfly12 profile and activate it by default for tests.
Implementation of
executeAsync
inHttpModelControllerClient
was added due to changes in WildFly Core in wildfly/wildfly-core@4713ebe#diff-bb461a3456601632260a50ddc3cc462cR628.