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

Consider refactoring the usages of clusterService.addListener(this) #37861

Closed
pgomulka opened this issue Jan 25, 2019 · 2 comments
Closed

Consider refactoring the usages of clusterService.addListener(this) #37861

pgomulka opened this issue Jan 25, 2019 · 2 comments
Assignees
Labels
:Core/Infra/Core Core issues without another label >refactoring team-discuss

Comments

@pgomulka
Copy link
Contributor

pgomulka commented Jan 25, 2019

I noticed few implementations of ClusterStateListener registering itself to a ClusterService within constructor using a possibly dangerous construct:

   clusterService.addListener(this);

This is explicitly publishing this reference before the object is constructed. That can in turn lead to race conditions and visibility problems .

It is well described by Brian Goetz in his articles or in his book
sample article: https://www.ibm.com/developerworks/library/j-jtp0618/index.html

That is also mentioned in a java language specification JLS 17.5 final field semantics

An object is considered to be completely initialized when its constructor finishes. A thread that can only see a reference to an object after that object has been completely initialized is guaranteed to see the correctly initialized values for that object's final fields.
The usage model for final fields is a simple one: Set the final fields for an object in that object's constructor; and do not write a reference to the object being constructed in a place where another thread can see it before the object's constructor is finished. If this is followed, then when the object is seen by another thread, that thread will always see the correctly constructed version of that object's final fields. It will also see versions of any object or array referenced by those final fields that are at least as up-to-date as the final fields are.

@pgomulka pgomulka added the :Core/Infra/Core Core issues without another label label Jan 25, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@pgomulka
Copy link
Contributor Author

pgomulka commented Feb 7, 2019

closing as the meta issue for refactoring has been created #38560

@pgomulka pgomulka closed this as completed Feb 7, 2019
dakrone added a commit to dakrone/elasticsearch that referenced this issue Oct 15, 2020
This change allows the setting for disabling the automatically installed stack templates (the
`logs-*-*`, `metrics-*-*`, and `synthetics-*-*` templates) to be changed dynamically.

As a byproduct, it also moves thes `IndexTemplateRegistry` to use an `initialize()` method so that
constructors are not tempted to use a `this` reference in the constructor (see elastic#37861 for more
information about why to avoid that).

Resolves elastic#62835
dakrone added a commit that referenced this issue Oct 16, 2020
* Make stack.templates.enabled a dynamic setting

This change allows the setting for disabling the automatically installed stack templates (the
`logs-*-*`, `metrics-*-*`, and `synthetics-*-*` templates) to be changed dynamically.

As a byproduct, it also moves thes `IndexTemplateRegistry` to use an `initialize()` method so that
constructors are not tempted to use a `this` reference in the constructor (see #37861 for more
information about why to avoid that).

Resolves #62835
dakrone added a commit to dakrone/elasticsearch that referenced this issue Oct 16, 2020
* Make stack.templates.enabled a dynamic setting

This change allows the setting for disabling the automatically installed stack templates (the
`logs-*-*`, `metrics-*-*`, and `synthetics-*-*` templates) to be changed dynamically.

As a byproduct, it also moves thes `IndexTemplateRegistry` to use an `initialize()` method so that
constructors are not tempted to use a `this` reference in the constructor (see elastic#37861 for more
information about why to avoid that).

Resolves elastic#62835
dakrone added a commit that referenced this issue Oct 16, 2020
* Make stack.templates.enabled a dynamic setting

This change allows the setting for disabling the automatically installed stack templates (the
`logs-*-*`, `metrics-*-*`, and `synthetics-*-*` templates) to be changed dynamically.

As a byproduct, it also moves thes `IndexTemplateRegistry` to use an `initialize()` method so that
constructors are not tempted to use a `this` reference in the constructor (see #37861 for more
information about why to avoid that).

Resolves #62835
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label >refactoring team-discuss
Projects
None yet
Development

No branches or pull requests

2 participants