fixed terminating stale threads on trap/proc_exit

[hritikgupta]

This is to terminate suspended threads in case an atomic wait occurs with a huge or indefinite (-1) timeout, followed by a proc exit or trap.

[loganek]

I don't see there's any lock in the notify_stale_threads callback. Should we rather have a lock on the shared_memory_list_lock mutex at the beginning of the function and release it at the end? Otherwise the map could be modified in the other thread making the traversal corrupted.

[hritikgupta]

the wasm_runtime_atomic_notify step in callback calls acquire_wait_info which tries to acquire shared_memory_list_lock, so I guess encapsulating it with that will cause issues?

[eloparco]

HashMap doesn't duplicate the key/values pairs, so wait_map_without_lock will point to the same elements of wait_map

[loganek]

I think you at least want to lock while you copy hashmaps. Also, what happens if after making a copy a new waiter is added to the map?

[hritikgupta]

@loganek While copying the original map, the original map already acquires a lock as it traverses. As inserts happen in the copy map, every insert locks the copy map. So I think the copy should be consistent? If you meant wrapping copy with shared_memory_list_lock -> addressed that.

What could be concerning here is that during the traversal of copy map, the original map could be updated. If I put a lock on the original map during traversal, the execution gets stuck because the callback steps in notify_stale_threads try to put a lock on the original map. (This is exactly why I had to create a copy). This might lead to some entries (newly added ones) in the original map not catered to at that point of time, but would be eventually handled, right?

[eloparco]

either way I'm still not sure it would be safe since you're not cloning the map but copying the pointers iiuc

[hritikgupta]

closing and reopening to reinitiate the build

[wenyongh]

When to free the memory? And why do nothing except LOG_ERROR when malloc/realloc memory failed?

[wenyongh]

How about traversing two times: the first time to get the total element count of wait_map, use it to allocate memory for AtomicWaitAddressArgs *args with size == offsetof(AtomicWaitAddressArgs, addr) + sizeof(void *) * total_elem_count , and then traverse the second time without allocating memory?

static void
xxx_callback(void *key, void *value, void *p_total_elem_count)
{
    *(uint32 *)p_total_elem_count = *(uint32 *)p_total_elem_count + 1;
}

os_mutex_lock(&shared_memory_list_lock);
total_elem_count = 0;
bh_hash_map_traverse(wait_map, xxx_callback, (void *)&total_elem_count);
allocate memory
bh_hash_map_traverse(wait_map, ..., args); /* set each data->addr[i] */
os_mutex_unlock(...)

[hritikgupta]

just wondering if there is any specific advantage of traversing two times? is it to ensure the final traversal of data->addr is safe i.e. we don't go out of bounds?

[loganek]

The purpose of that is to avoid multiple reallocations - knowing the size in advance will let you make only one allocation.

[eloparco]

Does this PR work in AOT mode?

Also, can you try it with the sample thread_termination.c after replacing

wasm-micro-runtime/samples/wasi-threads/wasm-apps/thread_termination.c

Line 34 in ee1871d

for (int i = 0; i < TIMEOUT_SECONDS; i++)

with a wait operation?

And try with TEST_TERMINATION_IN_MAIN_THREAD set to 0 and 1

wasm-micro-runtime/samples/wasi-threads/wasm-apps/thread_termination.c

Line 19 in ee1871d

#define TEST_TERMINATION_IN_MAIN_THREAD 1

Just to make sure that the base cases are covered.