Skip to content

qa: Use assert not BOOST_CHECK_* from multithreaded tests #14069

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 26, 2018
Merged

qa: Use assert not BOOST_CHECK_* from multithreaded tests #14069

merged 1 commit into from
Aug 26, 2018

Conversation

skeees
Copy link
Contributor

@skeees skeees commented Aug 26, 2018

Resolves thread sanitizer failure @MarcoFalke found in #14058

@maflcko maflcko changed the title Use assert not BOOST_CHECK_* from multithreaded tests qa: Use assert not BOOST_CHECK_* from multithreaded tests Aug 26, 2018
@maflcko maflcko added the Tests label Aug 26, 2018
@maflcko maflcko added this to the 0.17.0 milestone Aug 26, 2018
@maflcko maflcko merged commit 737670c into bitcoin:master Aug 26, 2018
maflcko pushed a commit that referenced this pull request Aug 26, 2018
Merge #14069: qa: Use assert not BOOST_CHECK_* from multithreaded tests
1117283 
737670c Use assert when running from multithreaded code as BOOST_CHECK_* are not thread safe (Jesse Cohen)

Pull request description:

  Resolves thread sanitizer failure @MarcoFalke found in #14058

Tree-SHA512: 24d86c2cdae21fee029ee4b06f633de4b3e655d3371d97f09db6fd3f24b29388a78110996712249c49e7fefa7bbc3d3c405d8b480382174831fe2f9a042a557e
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Aug 27, 2018
@skeees
Use assert when running from multithreaded code as BOOST_CHECK_* are …
cf3d7f9 
…not thread safe

Github-Pull: bitcoin#14069
Rebased-From: 737670c
@practicalswift practicalswift mentioned this pull request Aug 28, 2018
Merged
practicalswift
practicalswift reviewed Aug 28, 2018
View reviewed changes
src/test/scheduler_tests.cpp
@@ -138,11 +138,11 @@ BOOST_AUTO_TEST_CASE(singlethreadedscheduler_ordered)
// the callbacks should run in exactly the order in which they were enqueued
for (int i = 0; i < 100; ++i) {
queue1.AddToProcessQueue([i, &counter1]() {
BOOST_CHECK_EQUAL(i, counter1++);
assert(i == counter1++);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side effect in assertion: Fixed in #14088. Please review :-)

laanwj added a commit that referenced this pull request Aug 31, 2018
@laanwj
Merge #14088: tests: Don't assert(...) with side effects
709a15b 
ca1a093 Add regression test: Don't assert(...) with side effects (practicalswift)
4c3c9c3 Don't assert(...) with side effects (practicalswift)

Pull request description:

  Don't `assert(...)` with side effects.

  From the developer notes:

  > **Assertions should not have side-effects**
  >
  > Rationale: Even though the source code is set to refuse to compile with assertions disabled, having side-effects in assertions is unexpected and makes the code harder to understand

  These assertions were introduced quite recently (in #14069 which was merged two days ago) and since this is a recurring thing (see #13534 – "Don't assert(foo()) where foo() has side effects" from May) I added a simple regression test for the most obvious common side effect.

Tree-SHA512: be65db9d8d5d0f5752152ba73fe3fbb0531880f156d3cd7dfdf1752709979b63214e46ae64b1adbe1e09fa121278f4087f4ae49bff16cf8f5aec16ea6bde3650
@Bushstar Bushstar mentioned this pull request Sep 4, 2018
Merged
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Sep 14, 2018
@skeees
Use assert when running from multithreaded code as BOOST_CHECK_* are …
fa3fe5a 
…not thread safe

Github-Pull: bitcoin#14069
Rebased-From: 737670c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 27, 2021
@PastaPastaPasta
Merge bitcoin#14069: qa: Use assert not BOOST_CHECK_* from multithrea…
88a9682 
…ded tests

737670c Use assert when running from multithreaded code as BOOST_CHECK_* are not thread safe (Jesse Cohen)

Pull request description:

  Resolves thread sanitizer failure @MarcoFalke found in bitcoin#14058

Tree-SHA512: 24d86c2cdae21fee029ee4b06f633de4b3e655d3371d97f09db6fd3f24b29388a78110996712249c49e7fefa7bbc3d3c405d8b480382174831fe2f9a042a557e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 27, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14088: tests: Don't assert(...) with side effects
969de22 
ca1a093 Add regression test: Don't assert(...) with side effects (practicalswift)
4c3c9c3 Don't assert(...) with side effects (practicalswift)

Pull request description:

  Don't `assert(...)` with side effects.

  From the developer notes:

  > **Assertions should not have side-effects**
  >
  > Rationale: Even though the source code is set to refuse to compile with assertions disabled, having side-effects in assertions is unexpected and makes the code harder to understand

  These assertions were introduced quite recently (in bitcoin#14069 which was merged two days ago) and since this is a recurring thing (see bitcoin#13534 – "Don't assert(foo()) where foo() has side effects" from May) I added a simple regression test for the most obvious common side effect.

Tree-SHA512: be65db9d8d5d0f5752152ba73fe3fbb0531880f156d3cd7dfdf1752709979b63214e46ae64b1adbe1e09fa121278f4087f4ae49bff16cf8f5aec16ea6bde3650
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 28, 2021
@PastaPastaPasta
Merge bitcoin#14069: qa: Use assert not BOOST_CHECK_* from multithrea…
2fc4775 
…ded tests

737670c Use assert when running from multithreaded code as BOOST_CHECK_* are not thread safe (Jesse Cohen)

Pull request description:

  Resolves thread sanitizer failure @MarcoFalke found in bitcoin#14058

Tree-SHA512: 24d86c2cdae21fee029ee4b06f633de4b3e655d3371d97f09db6fd3f24b29388a78110996712249c49e7fefa7bbc3d3c405d8b480382174831fe2f9a042a557e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 28, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14088: tests: Don't assert(...) with side effects
b5a3283 
ca1a093 Add regression test: Don't assert(...) with side effects (practicalswift)
4c3c9c3 Don't assert(...) with side effects (practicalswift)

Pull request description:

  Don't `assert(...)` with side effects.

  From the developer notes:

  > **Assertions should not have side-effects**
  >
  > Rationale: Even though the source code is set to refuse to compile with assertions disabled, having side-effects in assertions is unexpected and makes the code harder to understand

  These assertions were introduced quite recently (in bitcoin#14069 which was merged two days ago) and since this is a recurring thing (see bitcoin#13534 – "Don't assert(foo()) where foo() has side effects" from May) I added a simple regression test for the most obvious common side effect.

Tree-SHA512: be65db9d8d5d0f5752152ba73fe3fbb0531880f156d3cd7dfdf1752709979b63214e46ae64b1adbe1e09fa121278f4087f4ae49bff16cf8f5aec16ea6bde3650
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@PastaPastaPasta
Merge bitcoin#14069: qa: Use assert not BOOST_CHECK_* from multithrea…
a6945f0 
…ded tests

737670c Use assert when running from multithreaded code as BOOST_CHECK_* are not thread safe (Jesse Cohen)

Pull request description:

  Resolves thread sanitizer failure @MarcoFalke found in bitcoin#14058

Tree-SHA512: 24d86c2cdae21fee029ee4b06f633de4b3e655d3371d97f09db6fd3f24b29388a78110996712249c49e7fefa7bbc3d3c405d8b480382174831fe2f9a042a557e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14088: tests: Don't assert(...) with side effects
5303574 
ca1a093 Add regression test: Don't assert(...) with side effects (practicalswift)
4c3c9c3 Don't assert(...) with side effects (practicalswift)

Pull request description:

  Don't `assert(...)` with side effects.

  From the developer notes:

  > **Assertions should not have side-effects**
  >
  > Rationale: Even though the source code is set to refuse to compile with assertions disabled, having side-effects in assertions is unexpected and makes the code harder to understand

  These assertions were introduced quite recently (in bitcoin#14069 which was merged two days ago) and since this is a recurring thing (see bitcoin#13534 – "Don't assert(foo()) where foo() has side effects" from May) I added a simple regression test for the most obvious common side effect.

Tree-SHA512: be65db9d8d5d0f5752152ba73fe3fbb0531880f156d3cd7dfdf1752709979b63214e46ae64b1adbe1e09fa121278f4087f4ae49bff16cf8f5aec16ea6bde3650
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@PastaPastaPasta
Merge bitcoin#14069: qa: Use assert not BOOST_CHECK_* from multithrea…
af66fc6 
…ded tests

737670c Use assert when running from multithreaded code as BOOST_CHECK_* are not thread safe (Jesse Cohen)

Pull request description:

  Resolves thread sanitizer failure @MarcoFalke found in bitcoin#14058

Tree-SHA512: 24d86c2cdae21fee029ee4b06f633de4b3e655d3371d97f09db6fd3f24b29388a78110996712249c49e7fefa7bbc3d3c405d8b480382174831fe2f9a042a557e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14088: tests: Don't assert(...) with side effects
60c7221 
ca1a093 Add regression test: Don't assert(...) with side effects (practicalswift)
4c3c9c3 Don't assert(...) with side effects (practicalswift)

Pull request description:

  Don't `assert(...)` with side effects.

  From the developer notes:

  > **Assertions should not have side-effects**
  >
  > Rationale: Even though the source code is set to refuse to compile with assertions disabled, having side-effects in assertions is unexpected and makes the code harder to understand

  These assertions were introduced quite recently (in bitcoin#14069 which was merged two days ago) and since this is a recurring thing (see bitcoin#13534 – "Don't assert(foo()) where foo() has side effects" from May) I added a simple regression test for the most obvious common side effect.

Tree-SHA512: be65db9d8d5d0f5752152ba73fe3fbb0531880f156d3cd7dfdf1752709979b63214e46ae64b1adbe1e09fa121278f4087f4ae49bff16cf8f5aec16ea6bde3650
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@PastaPastaPasta
Merge bitcoin#14069: qa: Use assert not BOOST_CHECK_* from multithrea…
47361e1 
…ded tests

737670c Use assert when running from multithreaded code as BOOST_CHECK_* are not thread safe (Jesse Cohen)

Pull request description:

  Resolves thread sanitizer failure @MarcoFalke found in bitcoin#14058

Tree-SHA512: 24d86c2cdae21fee029ee4b06f633de4b3e655d3371d97f09db6fd3f24b29388a78110996712249c49e7fefa7bbc3d3c405d8b480382174831fe2f9a042a557e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14088: tests: Don't assert(...) with side effects
689230b 
ca1a093 Add regression test: Don't assert(...) with side effects (practicalswift)
4c3c9c3 Don't assert(...) with side effects (practicalswift)

Pull request description:

  Don't `assert(...)` with side effects.

  From the developer notes:

  > **Assertions should not have side-effects**
  >
  > Rationale: Even though the source code is set to refuse to compile with assertions disabled, having side-effects in assertions is unexpected and makes the code harder to understand

  These assertions were introduced quite recently (in bitcoin#14069 which was merged two days ago) and since this is a recurring thing (see bitcoin#13534 – "Don't assert(foo()) where foo() has side effects" from May) I added a simple regression test for the most obvious common side effect.

Tree-SHA512: be65db9d8d5d0f5752152ba73fe3fbb0531880f156d3cd7dfdf1752709979b63214e46ae64b1adbe1e09fa121278f4087f4ae49bff16cf8f5aec16ea6bde3650
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@PastaPastaPasta
Merge bitcoin#14069: qa: Use assert not BOOST_CHECK_* from multithrea…
29578a6 
…ded tests

737670c Use assert when running from multithreaded code as BOOST_CHECK_* are not thread safe (Jesse Cohen)

Pull request description:

  Resolves thread sanitizer failure @MarcoFalke found in bitcoin#14058

Tree-SHA512: 24d86c2cdae21fee029ee4b06f633de4b3e655d3371d97f09db6fd3f24b29388a78110996712249c49e7fefa7bbc3d3c405d8b480382174831fe2f9a042a557e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14088: tests: Don't assert(...) with side effects
658d86e 
ca1a093 Add regression test: Don't assert(...) with side effects (practicalswift)
4c3c9c3 Don't assert(...) with side effects (practicalswift)

Pull request description:

  Don't `assert(...)` with side effects.

  From the developer notes:

  > **Assertions should not have side-effects**
  >
  > Rationale: Even though the source code is set to refuse to compile with assertions disabled, having side-effects in assertions is unexpected and makes the code harder to understand

  These assertions were introduced quite recently (in bitcoin#14069 which was merged two days ago) and since this is a recurring thing (see bitcoin#13534 – "Don't assert(foo()) where foo() has side effects" from May) I added a simple regression test for the most obvious common side effect.

Tree-SHA512: be65db9d8d5d0f5752152ba73fe3fbb0531880f156d3cd7dfdf1752709979b63214e46ae64b1adbe1e09fa121278f4087f4ae49bff16cf8f5aec16ea6bde3650
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@PastaPastaPasta
Merge bitcoin#14069: qa: Use assert not BOOST_CHECK_* from multithrea…
d9df413 
…ded tests

737670c Use assert when running from multithreaded code as BOOST_CHECK_* are not thread safe (Jesse Cohen)

Pull request description:

  Resolves thread sanitizer failure @MarcoFalke found in bitcoin#14058

Tree-SHA512: 24d86c2cdae21fee029ee4b06f633de4b3e655d3371d97f09db6fd3f24b29388a78110996712249c49e7fefa7bbc3d3c405d8b480382174831fe2f9a042a557e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14088: tests: Don't assert(...) with side effects
989e018 
ca1a093 Add regression test: Don't assert(...) with side effects (practicalswift)
4c3c9c3 Don't assert(...) with side effects (practicalswift)

Pull request description:

  Don't `assert(...)` with side effects.

  From the developer notes:

  > **Assertions should not have side-effects**
  >
  > Rationale: Even though the source code is set to refuse to compile with assertions disabled, having side-effects in assertions is unexpected and makes the code harder to understand

  These assertions were introduced quite recently (in bitcoin#14069 which was merged two days ago) and since this is a recurring thing (see bitcoin#13534 – "Don't assert(foo()) where foo() has side effects" from May) I added a simple regression test for the most obvious common side effect.

Tree-SHA512: be65db9d8d5d0f5752152ba73fe3fbb0531880f156d3cd7dfdf1752709979b63214e46ae64b1adbe1e09fa121278f4087f4ae49bff16cf8f5aec16ea6bde3650
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Jun 30, 2021
@Munkybooty
Merge bitcoin#14069: qa: Use assert not BOOST_CHECK_* from multithrea…
f547446 
…ded tests

737670c Use assert when running from multithreaded code as BOOST_CHECK_* are not thread safe (Jesse Cohen)

Pull request description:

  Resolves thread sanitizer failure @MarcoFalke found in bitcoin#14058

Tree-SHA512: 24d86c2cdae21fee029ee4b06f633de4b3e655d3371d97f09db6fd3f24b29388a78110996712249c49e7fefa7bbc3d3c405d8b480382174831fe2f9a042a557e
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Jun 30, 2021
@Munkybooty
Merge bitcoin#14069: qa: Use assert not BOOST_CHECK_* from multithrea…
1ff26a3 
…ded tests

737670c Use assert when running from multithreaded code as BOOST_CHECK_* are not thread safe (Jesse Cohen)

Pull request description:

  Resolves thread sanitizer failure @MarcoFalke found in bitcoin#14058

Tree-SHA512: 24d86c2cdae21fee029ee4b06f633de4b3e655d3371d97f09db6fd3f24b29388a78110996712249c49e7fefa7bbc3d3c405d8b480382174831fe2f9a042a557e
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Jul 1, 2021
@Munkybooty
Merge bitcoin#14069: qa: Use assert not BOOST_CHECK_* from multithrea…
d1d35d0 
…ded tests

737670c Use assert when running from multithreaded code as BOOST_CHECK_* are not thread safe (Jesse Cohen)

Pull request description:

  Resolves thread sanitizer failure @MarcoFalke found in bitcoin#14058

Tree-SHA512: 24d86c2cdae21fee029ee4b06f633de4b3e655d3371d97f09db6fd3f24b29388a78110996712249c49e7fefa7bbc3d3c405d8b480382174831fe2f9a042a557e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
@PastaPastaPasta
Merge bitcoin#14069: qa: Use assert not BOOST_CHECK_* from multithrea…
ee68324 
…ded tests

737670c Use assert when running from multithreaded code as BOOST_CHECK_* are not thread safe (Jesse Cohen)

Pull request description:

  Resolves thread sanitizer failure @MarcoFalke found in bitcoin#14058

Tree-SHA512: 24d86c2cdae21fee029ee4b06f633de4b3e655d3371d97f09db6fd3f24b29388a78110996712249c49e7fefa7bbc3d3c405d8b480382174831fe2f9a042a557e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14088: tests: Don't assert(...) with side effects
cd5a39c 
ca1a093 Add regression test: Don't assert(...) with side effects (practicalswift)
4c3c9c3 Don't assert(...) with side effects (practicalswift)

Pull request description:

  Don't `assert(...)` with side effects.

  From the developer notes:

  > **Assertions should not have side-effects**
  >
  > Rationale: Even though the source code is set to refuse to compile with assertions disabled, having side-effects in assertions is unexpected and makes the code harder to understand

  These assertions were introduced quite recently (in bitcoin#14069 which was merged two days ago) and since this is a recurring thing (see bitcoin#13534 – "Don't assert(foo()) where foo() has side effects" from May) I added a simple regression test for the most obvious common side effect.

Tree-SHA512: be65db9d8d5d0f5752152ba73fe3fbb0531880f156d3cd7dfdf1752709979b63214e46ae64b1adbe1e09fa121278f4087f4ae49bff16cf8f5aec16ea6bde3650
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@practicalswift practicalswift practicalswift left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
0.17.0
Development

Successfully merging this pull request may close these issues.
3 participants
@skeees @practicalswift @maflcko