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

Close all resources if doStart fails #9898

Closed
wants to merge 1 commit into from

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Feb 26, 2015

if we for instance can't bind a port we fail to stop the client bootstrap etc. To prevent this we can just call doStop() it has all the relevant checks in place.

@s1monw
Copy link
Contributor Author

s1monw commented Feb 26, 2015

we had a thread leaks reported on CI due to that http://build-us-00.elasticsearch.org/job/es_g1gc_master_metal/3929/

@s1monw
Copy link
Contributor Author

s1monw commented Feb 26, 2015

@spinscale can you take a look

@spinscale
Copy link
Contributor

LGTM, thinking if we should generalize this a bit more and have all AbstractLifeCycleComponents execute something like this to clean up

@s1monw
Copy link
Contributor Author

s1monw commented Mar 2, 2015

@spinscale I agreee but in general I think we should remove #stop completely, what do you think?

@spinscale
Copy link
Contributor

@s1monw agreed, we still have close() due to Releasable then

@s1monw
Copy link
Contributor Author

s1monw commented Mar 2, 2015

I'd love to also remove #start if possible 👯

@spinscale
Copy link
Contributor

hm. Wondering, how should that work with keeping in mind that constructors in guice apps should not throw exceptions and stuff (all done via listeners?)? I'd be fine with one method for stopping/starting each

@s1monw s1monw closed this in ed7f652 Mar 17, 2015
s1monw added a commit that referenced this pull request Mar 17, 2015
@clintongormley clintongormley changed the title [TRANSPORT] Close all resources if doStart fails Close all resources if doStart fails May 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants