Skip to content

Quick fix at_lookup race condition #139

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.

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

Merged
merged 5 commits into from
Feb 17, 2022
Merged

Conversation

gkc
Copy link
Contributor

@gkc gkc commented Feb 16, 2022

- What I did

  • Added use of a mutex into AtLookupImpl._process() to prevent race condition which surfaced during testing the mwc_demo end-to-end

- How I did it

  • Added dependency on mutex package
  • Created a Mutex for use by _process(); modified _process() to acquire the mutex to start with, and release it in a finally block

- How to verify it

  • DONE: Verified this fixed the issue for mwc_demo
  • DONE: Run all tests in at_client_sdk with a dependency override for at_lookup to this branch
  • DONE: test atmospherepro with this branch
  • DONE: Added a test to at_client_sdk/at_functional_test. (To see the test failing, add at_lookup: 3.0.7 to at_client_sdk/at_functional_test/pubspec.yaml)

- Description for the changelog

  • Added use of a mutex into AtLookupImpl._process() to prevent race condition

gkc added 2 commits February 16, 2022 17:03
…that, because of the way in which the send and receive operate, it is quite possible indeed very likely that if two GETs, for example, are issued ~simultaneously, then the results will be mixed up
…that, because of the way in which the send and receive operate, it is quite possible indeed very likely that if two GETs, for example, are issued ~simultaneously, then the results will be mixed up
@gkc gkc requested review from murali-shris, VJag, cconstab and nickelskevin and removed request for VJag February 16, 2022 17:23
@gkc
Copy link
Contributor Author

gkc commented Feb 16, 2022

@murali-shris Please review at your earliest convenience; given the severity of this bug, I'd like to merge to trunk asap - ideally tomorrow

I will add extra tests tomorrow morning (unit tests for at_lookup, as well as an end-to-end test)

In the interim, @purnimavenkatasubbu it would be great if you could locally run all of the tests in at_client_sdk, and maybe test AtmosherePro as well, in order to verify that this doesn't break something else. You will need

dependency_overrides:
  at_lookup:
    git:
      url: https://github.com/atsign-foundation/at_libraries.git
      path: at_lookup
      ref: quickFixAtLookupRaceCondition

@gkc
Copy link
Contributor Author

gkc commented Feb 16, 2022

Issue documented at #140

@gkc gkc changed the title Quick fix at lookup race condition Quick fix at_lookup race condition Feb 16, 2022
@purnimavenkatasubbu
Copy link
Member

Verified functional_tests of client_sdk locally and verified @mosphere_pro app pointing to the following in dependency_overrides
at_lookup:
git:
url: https://github.com/atsign-foundation/at_libraries.git
path: at_lookup
ref: quickFixAtLookupRaceCondition

Did not face any issues. Attaching POT's
atLookupTestLogs.txt

@mosphere_pro app recordings

sending_notifications.mp4
receiving_notifications.mp4
auth.mp4

@gkc
Copy link
Contributor Author

gkc commented Feb 17, 2022 via email

@gkc gkc merged commit b99cf51 into trunk Feb 17, 2022
@gkc gkc deleted the quickFixAtLookupRaceCondition branch February 17, 2022 21:43
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