Possible bug in multi_threaded_vector test

[alvinhochun]

While running the tests in my branch (which I'm pretty sure I haven't broke anything related to it), I got a failure in this test for the x86 Debug config: https://github.com/alvinhochun/cppwinrt/actions/runs/3255357951/jobs/5344593918

-------------------------------------------------------------------------------
multi_threaded_vector
-------------------------------------------------------------------------------
D:\a\cppwinrt\cppwinrt\test\test\multi_threaded_vector.cpp(455)
...............................................................................

D:\a\cppwinrt\cppwinrt\test\test\multi_threaded_vector.cpp(419): FAILED:
  CHECK( conditional_unbox<T>(t) == 1 )
with expansion:
  42 == 1

Perhaps there is a real race condition somewhere? I haven't checked it locally.

[kennykerr]

@dunhor wrote the multithreaded implementation and may have some insights.

[dunhor]

I'm not aware of anything nor have I kept up with changes made. I do know that @oldnewthing updated the tests to be more deterministic, so I'd imagine that the tests would no longer spuriously fail. I.e. the execution should effectively be single threaded.

Looking at the test, it seems as though it's supposed to wait for a collection_action::at, which should happen when Current is called. Therefore, that thread should halt execution until after the InsertAt call completes giving us one of two things: an iterator invalidation exception OR the value 42. From a simple read through, it's not entirely clear to me why this test would have ever succeeded, which likely means that I'm reading it incorrectly. Perhaps someone might see something I didn't.

[kennykerr]

The multithreaded vector tests are not very reliable.

https://github.com/microsoft/cppwinrt/actions/runs/3267638358/jobs/5373066605

[sylveon]

Yes, they often segfault when testing on my own machine

…

On Mon., Oct. 17, 2022, 14:52 Kenny Kerr, ***@***.***> wrote: The multithreaded vector tests are not very reliable. https://github.com/microsoft/cppwinrt/actions/runs/3267638358/jobs/5373066605 — Reply to this email directly, view it on GitHub <#1202 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABRELNTEIKJVH6WYCSS57E3WDWN5DANCNFSM6AAAAAARF3YFY4> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[dunhor]

So, I did a little bit of debugging and testing and realized that the tests rely on a "deadlock" timeout, which effectively means that there's no guarantee of determinism as I originally thought. Under normal circumstances, the execution looks like this:

1. Foreground tread: waits to be alerted
2. Background thread: calls First
3. Background thread: calls Current
4. Background thread: call to Current grabs the lock
5. Background thread: call to Current signals the foreground thread
6. Background thread: waits to be alerted
7. Foreground thread: calls InsertAt which blocks waiting for the lock
8. Background thread: times out and completes the call to Current
9. Foreground thread: completes the InsertAt call

It's possible for the foreground thread to time out (which began in 1 above) before 4 happens. If that's the case, InsertAt will "win" and the test will fail as it has here. Ideally it seems like we'd have more than one timeout value (10 ms is the default for all waits) so that step 1 waits longer. @oldnewthing can comment on the analysis/viability of such a proposal

[github-actions]

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.