Skip to content

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Oct 21, 2024

Description

Seen in EventsCanBePublishedNonBlockingConcurrent test results:

Assert.Equal() Failure: Values differ
Expected: 2
Actual:   1

It looks like events with code hitCount++ are run conccurent so the test will break if incremented at the same time. Use thread-safe increment.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
      • Link to aspire-docs issue:
    • No
Microsoft Reviewers: Open in CodeFlow

@JamesNK JamesNK added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Oct 21, 2024
@JamesNK JamesNK requested a review from mitchdenny as a code owner October 21, 2024 00:56
@JamesNK
Copy link
Member Author

JamesNK commented Oct 21, 2024

I found more cases of integers incremented inside events and callbacks. I think many of these aren't a problem, but there isn't harm in using the thread safe way to increment value in potentially multithreaded code.

@JamesNK JamesNK changed the title Fix EventsCanBePublishedNonBlockingConcurrent race condition Fix integer increment race condition Oct 21, 2024
@JamesNK JamesNK merged commit c8eff71 into main Oct 21, 2024
9 checks passed
@JamesNK JamesNK deleted the jamesnk/events-race-increment branch October 21, 2024 13:14
@github-actions github-actions bot locked and limited conversation to collaborators Nov 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants