Question regarding thread-safety of static fields in BoaviztAPI

[dpol1]

Hi @jnioche,

I've been exploring the codebase to better understand how the enrichment modules work, specifically looking at the BoaviztAPI class.

I noticed that the unknownInstanceTypes set is defined as a static field using a standard HashSet:
https://github.com/digitalpebble/spruce/blob/main/src/main/java/com/digitalpebble/spruce/modules/boavizta/BoaviztAPI.java#L35

private static Set<String> unknownInstanceTypes;

I have a question regarding the execution model: since Spark Executors typically run multiple tasks concurrently within the same JVM, wouldn’t these tasks share this static HashSet?

If so, is there a risk of ConcurrentModificationException (or race conditions) if multiple tasks try to add new unknown instance types at the same time, given that HashSet is not thread-safe?

I might be missing something about how the class is instantiated or isolated by the Spark executor, but if my understanding is correct, would it be safer to switch to ConcurrentHashMap.newKeySet()?

(I also noticed the same pattern in BoaviztAPIstatic.java, so the question applies there too).

Thanks for the clarification!

[jnioche]

thanks @davide954
I don't think it has much impact here, it is merely an optimisation: if the key is not found, we query the API and put it there. If another thread does the same at the same time, the worst thing that can happen is that they will both have queried the API twice. Better than blocking on the cache I think.

[dpol1]

Thanks @jnioche!

I totally agree regarding the cache – a few duplicate API calls are definitely better than blocking, so that trade-off makes perfect sense there.

However, my main concern was actually about the unknownInstanceTypes field (which is a standard HashSet).
Unlike the cache, if two threads try to .add() to a HashSet concurrently, it won't just result in duplicate work. It leads to structural corruption of the map.

This typically results in:

1. ConcurrentModificationException (crashing the task).
2. Infinite loops during map resizing (hanging the CPU at 100%), which I've seen happen in other Spark jobs with non-thread-safe statics.

Since ConcurrentHashMap.newKeySet() is highly optimized (non-blocking reads, lock striping for writes), using it for unknownInstanceTypes would prevent the crash risk with negligible performance impact.

Is that reasonable for you?

[jnioche]

unknownInstanceTypes acts as a cache which is why I called it so :-)

ConcurrentHashMap.newKeySet() is a relatively non-obtrusive fix, no reason not to do it! Feel free to open a PR, ideally covering other similar uses of Hashset in our code.

Thanks @davide954

[dpol1]

Thanks @jnioche I will proceed.

[jnioche]

Implemented in #148