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

Objects should not call eventBus.register(this) in constructors #155

Closed
JLLeitschuh opened this issue Nov 18, 2015 · 3 comments
Closed

Objects should not call eventBus.register(this) in constructors #155

JLLeitschuh opened this issue Nov 18, 2015 · 3 comments
Assignees
Milestone

Comments

@JLLeitschuh
Copy link
Member

Problem

Registering an object to the event bus inside of a constructor call can result in a subscriber being called prior to all of the fields in the class being fully constructed. This can lead to null pointer exceptions for fields that are final and should have been initialized in the classes constructor.

Relavent Gitter Discussion Discovery of problem and discussion of how to fix it.

Guava Google Groups Discussion with Dev

Don't publish the "this" reference during construction

One of the mistakes that can introduce a data race into your class is to expose the this reference to another thread before the constructor has completed. Sometimes the reference is explicit, such as directly storing this in a static field or collection, but other times it can be implicit, such as when you publish a reference to an instance of a non-static inner class in a constructor. Constructors are not ordinary methods -- they have special semantics for initialization safety. An object is assumed to be in a predictable, consistent state after the constructor has completed, and publishing a reference to an incompletely constructed object is dangerous. Listing 2 shows an example of introducing this sort of race condition into a constructor. It may look harmless, but it contains the seeds of serious concurrency problems.
-Brian Goetz

Resolution

This can be resolved by ensuring that constructors never call a register method

@ThomasJClark ThomasJClark changed the title Notes: Event Bus should not call register on this in constructors Notes: Objects should not call eventBus.register(this) in constructors Nov 18, 2015
@ThomasJClark ThomasJClark changed the title Notes: Objects should not call eventBus.register(this) in constructors Objects should not call eventBus.register(this) in constructors Nov 18, 2015
@JLLeitschuh
Copy link
Member Author

Resolution, this will be handled with dependency injection.

@ThomasJClark
Copy link
Contributor

Now that our exception dialog at least doesn't stack overflow when there's a problem with the GUI thread, here's a full stacktrace for the fabled NullPointerException (just so it's actually documented somewhere)

Identical traces seem to be pretty common in JavaFX applications if you do some Googling, but the actual cause is kind of hard to track down. Avoiding unsafe things like leaking this and modifying the UI outside of Platform.runLater blocks is probably our best bet.

java.lang.NullPointerException
    at javafx.scene.Scene$ScenePulseListener.synchronizeSceneNodes(Scene.java:2289)
    at javafx.scene.Scene$ScenePulseListener.pulse(Scene.java:2419)
    at com.sun.javafx.tk.Toolkit.lambda$runPulse$30(Toolkit.java:314)
    at com.sun.javafx.tk.Toolkit$$Lambda$310/747188721.run(Unknown Source)
    at java.security.AccessController.doPrivileged(Native Method)
    at com.sun.javafx.tk.Toolkit.runPulse(Toolkit.java:313)
    at com.sun.javafx.tk.Toolkit.firePulse(Toolkit.java:340)
    at com.sun.javafx.tk.quantum.QuantumToolkit.pulse(QuantumToolkit.java:525)
    at com.sun.javafx.tk.quantum.QuantumToolkit.pulse(QuantumToolkit.java:505)
    at com.sun.javafx.tk.quantum.QuantumToolkit.lambda$runToolkit$400(QuantumToolkit.java:334)
    at com.sun.javafx.tk.quantum.QuantumToolkit$$Lambda$45/429586642.run(Unknown Source)
    at com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:95)
    at com.sun.glass.ui.gtk.GtkApplication._runLoop(Native Method)
    at com.sun.glass.ui.gtk.GtkApplication.lambda$null$48(GtkApplication.java:139)
    at com.sun.glass.ui.gtk.GtkApplication$$Lambda$41/1327763628.run(Unknown Source)
    at java.lang.Thread.run(Thread.java:745)

@JLLeitschuh
Copy link
Member Author

This should be resolved with dependency injection #215

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants