Skip to content

Conversation

@ZedThree
Copy link
Member

@ZedThree ZedThree commented Mar 3, 2020

I've not actually managed to make a test for MsgStack that fails without this change and passes with it. For example:

TEST(MsgStackTest, ThreadSafety) {
  MsgStack msg_stack;

  const std::string expected_dump = "====== Back trace ======\n";

  BOUT_OMP(parallel for)
  for (int i = 0; i < 1000; i++) {
    int id = msg_stack.push("Message", i);
    msg_stack.pop(id);
  }
  auto dump = msg_stack.getDump();
  EXPECT_EQ(dump, expected_dump);
}

looks like something this change should fix, but doesn't.

@johnomotani Any chance you've got an example of something that was failing before?

We could use standard library std::lock_guard with a custom mutex here, that might also let us be thread safe for other types of threads, if anyone is ever likely to use them.

Makes 'count', which is incremented in the PetscLib constructor and
decremented in the destructor, thread-safe.
Surround all uses of 'count' with BOUT_OMP(critical).
Also name the critical regions 'PetscLib' to ensure they do not conflict
with other critical regions.
Use same name 'MsgStack' for BOUT_OMP(critical(MsgStack)) declarations
where 'position' is used. Ensures that reads and writes of 'position' by
different threads in different methods of MsgStack cannot conflict.
@ZedThree ZedThree added this to the BOUT-5.0 milestone Mar 3, 2020
@ZedThree ZedThree requested a review from johnomotani March 3, 2020 17:37
@ZedThree ZedThree mentioned this pull request Mar 3, 2020
@ZedThree ZedThree merged commit 8dfdc33 into next Mar 6, 2020
@ZedThree ZedThree deleted the msgstack-petsclib-thread-safety branch March 6, 2020 10:39
@johnomotani
Copy link
Contributor

@johnomotani Any chance you've got an example of something that was failing before?

I don't think I ever had a test that was failing because of MsgStack, it was just that TSan was complaining about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants