MT-safety of HistoOnEvent example

[magicwerk]

I use

https://github.com/btraceio/btrace/blob/master/btrace-dist/src/main/resources/samples/HistoOnEvent.java

as base for collecting information for classes.

    public static void onnewObject(@Self Object obj) {
        String cn = name(classOf(obj));
        AtomicInteger ai = get(histo, cn);
        if (ai == null) {
            ai = newAtomicInteger(1);
            put(histo, cn, ai);
        } else {
            incrementAndGet(ai);
        }
    }

However I have the problem that the results are not accurate due to MT issues. If I add log statements, I get the following output:

start of onnewObject for Thread[Thread-6,5,main]
newAtomicInteger org.apache.commons.lang3.builder.EqualsBuilder

start Thread[Thread-7,5,main]
newAtomicInteger org.apache.commons.lang3.builder.EqualsBuilder

end of onnewObject for Thread[Thread-7,5,main]

end of onnewObject for Thread[Thread-6,5,main]

Obviously access to onnewObject is not synchronized by BTrace itself and I cannot add synchronized myself because it's prohibited.

The statements

        AtomicInteger ai = get(histo, cn);
        if (ai == null) {
            ai = newAtomicInteger(1);

use check-then-act which produces an incorrect result in the case shown.

The map implementations seems to be provided by BTraceMap, but I did not spot any syncronization. Does BTrace provides some synchronization I missed to guarantee that accessing a single HashMap from several threads is safe?

What do you propose to solve the check-then-act problem? If BTrace internally would synchronizes each access to the Map, it could enough to offer methods like computeIfAbsent. If synchronization is missing, probably access to ConcurrentHashMap would be needed.

[jbachorik]

Unfortunately, this is a complex problem :( For anything like 'computeIfAbsent' we would need to have support for lambdas and that is not trivial (probably doable, but requiring carefully designing the APIs and then implementation, of course).

If you need the synchronization now you can do @BTrace(trusted = true) to turn the safety verification off and then it is up to you to make sure you are not going to deadlock the traced app.

[jbachorik]

Unfortunately, this is a complex problem :( For anything like 'computeIfAbsent' we would need to have support for lambdas and that is not trivial (probably doable, but requiring carefully designing the APIs and then implementation, of course).

Having said that, I have been pondering nicer APIs with Java 8 features for some time (eg. we could do iterations on collections as they would be guaranteed to be bounded etc.)

[magicwerk]

First of all, thanks for you always answering so promptly!

If I interpret your answer correctly, it seems that no synchronization happens at all between methods like onnewObject. This would mean that access from several threads could not only produce false results in my example, but also crash the traced applications if unsafe MT access wrangles up some internal data structures.

So the HistoOnEvent example would be correct as the used Swing class is intended to use in a single threaded way, but is unsafe for general use. Therefore I think it would be good to add this information to the javadoc.

Then probably a more general example with @BTrace(trusted=true) should be added showing MT-safety can be achieved in a general way.

I was also looking for general documentation about the MT behavior of the traced code, but this seems to be missing. Also the mentioned annotation parameters of @Btrace are not documented in the Wiki.

I was also looking for the source code in the btrace-client-2.2.-sources.jar, but the JAR does not contain the sources. As this JARs is downloaded by Maven to provide sources, it would be good if they would be contained.

[jbachorik]

First of all, thanks for you always answering so promptly!

Not at all. It is refreshing to have engaging users :)

If I interpret your answer correctly, it seems that no synchronization happens at all between methods like onnewObject. This would mean that access from several threads could not only produce false results in my example, but also crash the traced applications if unsafe MT access wrangles up some internal data structures.

Unfortunately, this is true.

So the HistoOnEvent example would be correct as the used Swing class is intended to use in a single threaded way, but is unsafe for general use. Therefore I think it would be good to add this information to the javadoc.

Yes, I will try to get some time this week to fix this. Or you can do a PR for this if you feel like it.

Then probably a more general example with @btrace(trusted=true) should be added showing MT-safety can be achieved in a general way.

The same as for the previous point.

I was also looking for general documentation about the MT behavior of the traced code, but this seems to be missing. Also the mentioned annotation parameters of @btrace are not documented in the Wiki.

Yes, the documenatation is rather sparse :(

I was also looking for the source code in the btrace-client-2.2.-sources.jar, but the JAR does not contain the sources. As this JARs is downloaded by Maven to provide sources, it would be good if they would be contained.

I will verify this. I was relying on the gradle tasks doing their job but there might be some problem in config.

[github-actions[bot]]

Stale issue message