Skip to content

Conversation

@pbaranchikov
Copy link
Contributor

@pbaranchikov pbaranchikov commented Sep 15, 2016

As executorService could be shut down from another thread, it is not suficient to synchronize its lazy initialization, but it is also required to synchronized its usage in order to prevent another thread from shutting down the executorService right before its usage.

As fix is thread-safety oriented, there is not tests intoduced to cover the case.

fixes #132

@pivotal-issuemaster
Copy link

@pbaranchikov Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@pbaranchikov Thank you for signing the Contributor License Agreement!

@spencergibb
Copy link
Member

I'm not keen on an optimization for something that isn't really supported.

While running multiple instances of spring boot simultaneously (for testing purposes), sometimes, I can see the following error:

Thoughts @dsyer or @ryanjbaxter?

@pbaranchikov
Copy link
Contributor Author

@spencergibb , the change fixes the error in thread safety. I do not know other scenarios, that could cause creation of multiple instances of InetUtils. If there are the scenarios they can also cause the error reported.

If you are not supporting creation of multiple instances of InetUtils, why does the class use static member executorService? Why introduce any static variable, that is used locally from instance, if you are not expecting multiple instances? This leads me to the conclusion, that by design InetUtils expect multiple instances to be created.

Copy link
Member

@spencergibb spencergibb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

executorService doesn't need to be static, therefor it's creation can happen in the constructor and doesn't need to be syncronized at all. If you'd like to update your PR to make that happen, great, otherwise close and we'll make it happen.

@pbaranchikov
Copy link
Contributor Author

@spencergibb I agree with you approach to make executorService non-static. Please take a look at new version of PR.

@spencergibb spencergibb merged commit 329c67f into spring-cloud:master Oct 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RejectedExecutionException in InetUtils while running multiple spring-boot instances

3 participants