Safe to use (non-thread-safe) mutableMap in suspend function?

Question

I'm learning Coroutines in Kotlin and I have a piece of code that looks like this (see below).

My friend says that the mutableMapOf is LinkedHashMap, which is not thread safe. The argument is that the suspend function may be run by different threads, and thus LinkedHashMap is unsuitable.

1. Is it safe to use a simple mutable map here or is ConcurrentMap needed?
2. When a suspend function is suspended, can it be resumed and executed by another thread?
3. Even if (2) is possible, is there "happens-before/ happens-after" guarantee that ensures all the variables (and the underlying object contents) are deep synchronized from main memory before the new thread takes over?

Here's a simplified version of the code:

class CoroutineTest {

  private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default)

  suspend fun simpleFunction(): MutableMap<Int,String> { 

    val myCallResults = mutableMapOf<Int,String>()

    val deferredCallResult1 = scope.async {
         //make rest call get string back
    }
    val deferredCallResult2 = scope.async {
         //make rest call get string back
    }
    ...
    myCallResults.put( 1, deferredCallResult1.await() )
    myCallResults.put( 2, deferredCallResult2.await() )
    ...
   
    return myCallResults
  }
}

Thanks in advance!
PS. I ran this code with much more async call results and had no problem; all call results are accounted for. But that can be inconclusive which is why I ask.

Answer 1

1. No, it is not safe to use a single mutableMapOf() from multiple coroutines.
2. You understand suspending incorrectly. This is not function that is suspended. The coroutine running in the function could suspend. From this perspective suspending functions aren't really different than normal functions - they could be executed by many coroutines at the same time and all of them will work concurrently.

But... there is nothing wrong with your code for another reason. This mutable map is a local variable, so it is only available to the coroutine/thread that created it. Therefore, it is not accessed concurrently at all. It would be different if the map would be a property of CoroutineTest - then it might mean you need to use ConcurrentMap.

Updated

After reading all comments I believe I have a better understanding of your (or your friend) concerns, so I can provide a more accurate answer.

Yes, after suspending a coroutine it can resume from another thread, so coroutines make possible that some part of a function will be executed by one thread and other part will be executed by another thread. In your example it is possible that put(1 and put(2 will be invoked from two different threads.

However, saying that LinkedHashMap is not thread-safe doesn't mean, that it has to be always accessed by the same thread. It can be accessed by multiple threads, but not at the same time. One thread need to finish performing changes to the map and only then another thread can perform its modifications.

Now, in your code async { } blocks can work in parallel to each other. They can also work in parallel to the outer scope. But the contents of each of them works sequentially. put(2 line can only be executed after put(1 fully finishes, so the map is not accessed by multiple threads at the same time.

As stated earlier, it would be different if the map would be stored e.g. as a property, simpleFunction() would modify it and this function would be invoked multiple times in parallel - then each invocation would try to modify it at the same time. It would be also different if async operations would modify myCallResults directly. As I said, async blocks run in parallel to each other, so they could modify the map at the same time. But since you only return a result from async blocks and then modify the map from a single coroutine (from outer scope), the map is accessed sequentially, not concurrently.

Answer 2

Since the map is local to the suspend function, it is safe to use a non-thread-safe implementation. It is possible that different threads will be working with the map between different suspend function calls (in this case the await() calls), but there is guaranteed happens-before/happens-after within the suspend function.

If your map were declared outside the suspend function and accessed via a property, then there could be simultaneous calls to this function and you would be concurrently modifying it, which would be a problem.