4 ms·

(Author here) This blog post is actually part of a series to write profilers from scratch (https://mostlynerdless.de/blog/tag/writing-a-profiler-from-scratch/ h

(Author here) This blog post is actually part of a series to write profilers from scratch (https://mostlynerdless.de/blog/tag/writing-a-profiler-from-scratch/ https://mostlynerdless.de/blog/tag/writing-a-profiler-from-s...) and I'm looking forward to writing the next installment, in the next few days. I write a blog post on this and related in-depth profiling topics every two weeks (you can follow me on social media https://twitter.com/parttimen3rd https://twitter.com/parttimen3rd and https://mastodon.social/@parttimenerd https://mastodon.social/@parttimenerd to get notified).

_old_dude_ 4y ago
Hi. I believe the code of the constructor of Profiler is not thread safe, it has a publication problem. The method onEnd can be called by the profiler thread while the fields options and store are not fully initialized. From another thread POV, a final field is only fully initialized after the call to the constructor not inside the call to the constructor.
- parttimenerd 4y ago
  Thanks. But this should only be a problem if the JVM terminates too fast, or am I wrong?
  - _old_dude_ 4y ago
    For the context, I'm re-reading Java concurrency in Practice now so I see publication issues everywhere. I believe the issue is between the profiler thread and the shutdown hook thread. Both will run concurrently because the profiler thread is marked deamon. So the shutdown hook thread can see the Profiler fields not fully initialized. The call to addShutdownHook() should be done outside of the Profiler constructor.
    - layer8 4y ago
      You are correct. The solution is to use a static constructor method, like this: public static Profiler newInstance(Options options) { Profiler profiler = new Profiler(options); Runtime.getRuntime().addShutdownHook(new Thread(profiler::onEnd)); return profiler; } private Profiler(Options options) { this.options = options; this.store = new Store(options.getFlamePath()); } In principle you can also use chained constructors: public Profiler(Options options) { this(options, null); Runtime.getRuntime().addShutdownHook(new Thread(this::onEnd)); // okay to leak this here } private Profiler(Options options, Void dummy) { this.options = options; this.store = new Store(options.getFlamePath()); } (cf. https://stackoverflow.com/a/35169705/623763 https://stackoverflow.com/a/35169705/623763)
      - parttimenerd 4y ago
        Thank you. I fixed the code in the GitHub repository but kept the code on my blog the same, with a disclaimer regarding its problems.
  - aardvark179 4y ago
    It would have to be very quick as it would need the thread to be started before your object has finished being constructed. So it seems unlikely. What's not clear to me from a quick reading of the code is whether the store is actually correct. Although it is only being filled by a single thread that isn't the one that is going to read it on shutdown and I'm not clear on whether there is anything forcing a barrier before the onExit thread has starts reading the hash maps on the nodes. Actually, is there anything stopping the sampler from taking another sample while onExit is running?
    - brewmarche 4y ago
      Would `volatile` help with the barrier? Re the multi-threading issue, it should be possible to stop the profiler thread in the hook, if we keep a reference to the thread, no?
      - aardvark179 4y ago
        The fields are final, so they can't be volatile. :-) Using one of the fence methods on VarHandles would ensure the correct ordering. To stop the profiler thread it would be best to provide a controlled method of doing it, there's a lot of way it can be implemented but you probably want to signal it in some way and then do a join() to wait while it shuts down cleanly.
        parttimenerd 4y ago
        Thanks, I implemented it using a boolean flag.
- benmmurphy 4y ago
  doesn't `Runtime.addShutdownHook` have a synchronisation point. like all writes need to have occurred before `ApplicationShutdownHooks.add` has been called so there is no publish problem. https://github.com/openjdk/jdk/blob/2fa09333ef0ac2dc1e44292f8d45d4571cb22cca/src/java.base/share/classes/java/lang/Runtime.java#L69 https://github.com/openjdk/jdk/blob/2fa09333ef0ac2dc1e44292f8d45d4571cb22cca/src/java.base/share/classes/java/lang/ApplicationShutdownHooks.java#L65 maybe I'm missing something that is java specific with the way constructors work. I'm guessing even without the synchronisation point java lets you share objects in a way that would normally be considered unsafe as long as all the fields are final.

_old_dude_ 4y ago

Hi. I believe the code of the constructor of Profiler is not thread safe, it has a publication problem. The method onEnd can be called by the profiler thread while the fields options and store are not fully initialized. From another thread POV, a final field is only fully initialized after the call to the constructor not inside the call to the constructor.

parttimenerd 4y ago
Thanks. But this should only be a problem if the JVM terminates too fast, or am I wrong?
- _old_dude_ 4y ago
  For the context, I'm re-reading Java concurrency in Practice now so I see publication issues everywhere. I believe the issue is between the profiler thread and the shutdown hook thread. Both will run concurrently because the profiler thread is marked deamon. So the shutdown hook thread can see the Profiler fields not fully initialized. The call to addShutdownHook() should be done outside of the Profiler constructor.
  - layer8 4y ago
    You are correct. The solution is to use a static constructor method, like this: public static Profiler newInstance(Options options) { Profiler profiler = new Profiler(options); Runtime.getRuntime().addShutdownHook(new Thread(profiler::onEnd)); return profiler; } private Profiler(Options options) { this.options = options; this.store = new Store(options.getFlamePath()); } In principle you can also use chained constructors: public Profiler(Options options) { this(options, null); Runtime.getRuntime().addShutdownHook(new Thread(this::onEnd)); // okay to leak this here } private Profiler(Options options, Void dummy) { this.options = options; this.store = new Store(options.getFlamePath()); } (cf. https://stackoverflow.com/a/35169705/623763 https://stackoverflow.com/a/35169705/623763)
    - parttimenerd 4y ago
      Thank you. I fixed the code in the GitHub repository but kept the code on my blog the same, with a disclaimer regarding its problems.
- aardvark179 4y ago
  It would have to be very quick as it would need the thread to be started before your object has finished being constructed. So it seems unlikely. What's not clear to me from a quick reading of the code is whether the store is actually correct. Although it is only being filled by a single thread that isn't the one that is going to read it on shutdown and I'm not clear on whether there is anything forcing a barrier before the onExit thread has starts reading the hash maps on the nodes. Actually, is there anything stopping the sampler from taking another sample while onExit is running?
  - brewmarche 4y ago
    Would `volatile` help with the barrier? Re the multi-threading issue, it should be possible to stop the profiler thread in the hook, if we keep a reference to the thread, no?
    - aardvark179 4y ago
      The fields are final, so they can't be volatile. :-) Using one of the fence methods on VarHandles would ensure the correct ordering. To stop the profiler thread it would be best to provide a controlled method of doing it, there's a lot of way it can be implemented but you probably want to signal it in some way and then do a join() to wait while it shuts down cleanly.
      - parttimenerd 4y ago
        Thanks, I implemented it using a boolean flag.
benmmurphy 4y ago
doesn't `Runtime.addShutdownHook` have a synchronisation point. like all writes need to have occurred before `ApplicationShutdownHooks.add` has been called so there is no publish problem. https://github.com/openjdk/jdk/blob/2fa09333ef0ac2dc1e44292f8d45d4571cb22cca/src/java.base/share/classes/java/lang/Runtime.java#L69 https://github.com/openjdk/jdk/blob/2fa09333ef0ac2dc1e44292f8d45d4571cb22cca/src/java.base/share/classes/java/lang/ApplicationShutdownHooks.java#L65 maybe I'm missing something that is java specific with the way constructors work. I'm guessing even without the synchronisation point java lets you share objects in a way that would normally be considered unsafe as long as all the fields are final.