fix: race condition in keploy record #1867

dxtym · 2024-05-07T19:04:19Z

Related Issue

Race condition with testCount while exiting keploy record

Closes: #1851

Describe the changes you've made

Put sync.Mutex locks over the sharing value so that concurrent activities don't cause race condition. Add sync.waitGroup to control the flow of actions between the main and sub gorountines.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Code style update (formatting, local variables)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Please let us know if any test cases are added

No additional tests. Only the build procedure at #1851

Describe if there is any unusual behaviour of your code(Write `NA` if there isn't)

NA

Checklist:

My code follows the style guidelines of this project.
I have performed a self-review of my own code.
I have commented my code, particularly in hard-to-understand areas.
I have made corresponding changes to the documentation.
My changes generate no new warnings.
I have added tests that prove my fix is effective or that my feature works.
New and existing unit tests pass locally with my changes.

Screenshots (if any)

Original	Updated

github-actions · 2024-05-07T19:04:34Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

github-actions

Thank you and congratulations 🎉 for opening your very first pull request in keploy

dxtym · 2024-05-07T19:05:32Z

Singed-off-by: Dilmurod Abdusamadov [email protected]

dxtym · 2024-05-07T19:19:51Z

I have read the CLA Document and I hereby sign the CLA

slayerjain

Hi @dxtym , Thanks for the PR! I have left some comments and I think @gouravkrosx / @charankamarapu should give more context for a better solution.

slayerjain · 2024-05-09T14:01:45Z

pkg/service/record/record.go

 var mockCountMap = make(map[string]int)

 // defering the stop function to stop keploy in case of any error in record or in case of context cancellation
 defer func() {
 select {
 case <-ctx.Done():
+ waitGroup.Wait()
+ mutex.Lock()
 r.telemetry.RecordedTestSuite(newTestSetID, testCount, mockCountMap)


I might be missing something but not really sure if this defer could be called concurrently 🤔

aah, so its the testCount probably..

I think in the defer function if we wait for the other goroutines to complete/cancel - this race condition can be better resolved. Maybe we can move the runAppErrGrp.Wait() in the defer to the top.

@charankamarapu @gouravkrosx what do you think?

@dxtym , we can move the r.telemetry.RecordedTestSuite(newTestSetID, testCount, mockCountMap) to the bottom of the defer, this way we are waiting for all the go routines to complete their execution.

Thank you for the comments @slayerjain @gouravkrosx! I moved the r.telemetry.RecordedTestSuite(newTestSetID, testCount, mockCountMap) to the bottom and removed mutex and waitGroups. Did I get your feedback right?

@dxtym yes, I think you've made the changes according to the feedback.

It seems like ctx cancellation is not working correctly based on the this pipeline - https://github.com/keploy/keploy/actions/runs/9034964803/job/24829105617?pr=1867

It seems to be stuck on the last line:

I tried removing waitGroup from my previous solution, and the binary worked the same way. Perhaps we could try that

* [Feature]: Create a normalisation command in keploy CLI (keploy#1589) * Initialised Normalise Command Signed-off-by: Akash <[email protected]> * Path init Signed-off-by: Akash <[email protected]> * Successfully found the expected and actual responses for failing tests Signed-off-by: Akash <[email protected]> * Files are being updated but structs are not proper: func WriteTestcase doesnt match yaml file Signed-off-by: Akash <[email protected]> * Updating testcase files successfully Signed-off-by: Akash <[email protected]> * Added flags to collect test-set and test-cases from user Signed-off-by: Akash <[email protected]> * Fixed lint errors Signed-off-by: Akash <[email protected]> * Fixed lint errors Signed-off-by: Akash <[email protected]> * Refactored PR for Keploy v2 and fixed merge conflicts Signed-off-by: Akash Singh <[email protected]> * Normalise Test cases Signed-off-by: Akash Singh <[email protected]> * Addressed comments and refactored changes Signed-off-by: Akash Singh <[email protected]> * Fixed Lint Signed-off-by: Akash Singh <[email protected]> * Fixed Lint Signed-off-by: Akash Singh <[email protected]> * Refactoring and Adding Additional Flags Signed-off-by: Akash Singh <[email protected]> * Added Normalise to replay service Signed-off-by: Akash Singh <[email protected]> * Fixed lint Signed-off-by: Akash Singh <[email protected]> --------- Signed-off-by: Akash <[email protected]> Signed-off-by: Akash Singh <[email protected]> Co-authored-by: Animesh Pathak <[email protected]> * feat: add normalization Signed-off-by: charankamarapu <[email protected]> * fix: bugs Signed-off-by: charankamarapu <[email protected]> * fix: bugs Signed-off-by: charankamarapu <[email protected]> * fix: bugs Signed-off-by: charankamarapu <[email protected]> * fix: resolve comments Signed-off-by: charankamarapu <[email protected]> --------- Signed-off-by: Akash <[email protected]> Signed-off-by: Akash Singh <[email protected]> Signed-off-by: charankamarapu <[email protected]> Co-authored-by: Sky Singh <[email protected]> Co-authored-by: Animesh Pathak <[email protected]> Signed-off-by: dxtym <[email protected]>

Signed-off-by: dxtym <[email protected]>

Signed-off-by: Amogh Umesh <[email protected]> Signed-off-by: dxtym <[email protected]>

Co-authored-by: Sarthak Shyngle <[email protected]> Signed-off-by: dxtym <[email protected]>

* chore: add docker ip log and remove a switch Signed-off-by: Shubham Jain <[email protected]> * fix: check container ip before starting tests Signed-off-by: Shubham Jain <[email protected]> --------- Signed-off-by: Shubham Jain <[email protected]> Signed-off-by: dxtym <[email protected]>

ougoung -> outgoing Signed-off-by: Priyansh Agrawal <[email protected]> Co-authored-by: Sarthak Shyngle <[email protected]> Signed-off-by: dxtym <[email protected]>

Signed-off-by: dxtym <[email protected]>

* fix: folder permissions Signed-off-by: charankamarapu <[email protected]> * fix: folder permissions Signed-off-by: charankamarapu <[email protected]> * fix: unset umask Signed-off-by: charankamarapu <[email protected]> * doc: add comment Signed-off-by: charankamarapu <[email protected]> * doc: add comment Signed-off-by: charankamarapu <[email protected]> --------- Signed-off-by: charankamarapu <[email protected]> Signed-off-by: dxtym <[email protected]>

Signed-off-by: dxtym <[email protected]>

github-actions bot reviewed May 7, 2024

View reviewed changes

dxtym force-pushed the fix-race-cond branch from a8308a9 to 7a56866 Compare May 7, 2024 19:08

slayerjain added a commit that referenced this pull request May 7, 2024

@dxtym has signed the CLA from Pull Request #1867

175f09f

nehagup requested a review from slayerjain May 9, 2024 13:56

slayerjain requested changes May 9, 2024

View reviewed changes

dxtym force-pushed the fix-race-cond branch from ee3b27f to 279dc0c Compare May 10, 2024 16:02

charankamarapu and others added 11 commits May 13, 2024 23:02

fix: race condition in keploy record

9a3f805

Signed-off-by: dxtym <[email protected]>

Make noise fields case insensitive (keploy#1872)

2a921f7

Signed-off-by: Amogh Umesh <[email protected]> Signed-off-by: dxtym <[email protected]>

chore: parameterise binary to docker logic (keploy#1874)

a7c3980

Co-authored-by: Sarthak Shyngle <[email protected]> Signed-off-by: dxtym <[email protected]>

Fix typo in cli/mock.go (keploy#1860)

8147d0c

ougoung -> outgoing Signed-off-by: Priyansh Agrawal <[email protected]> Co-authored-by: Sarthak Shyngle <[email protected]> Signed-off-by: dxtym <[email protected]>

fix: move recordtestsuite down

708485c

Signed-off-by: dxtym <[email protected]>

fix: use mutex to resolve race condition

42e1e95

Signed-off-by: dxtym <[email protected]>

add: bypass rules filter (keploy#1837)

123b317

Signed-off-by: dxtym <[email protected]>

fix: use mutex to resolve race condition

9c79f90

Signed-off-by: dxtym <[email protected]>

dxtym force-pushed the fix-race-cond branch from b52adb2 to 9c79f90 Compare May 13, 2024 18:03

Merge branch 'main' into fix-race-cond

3151d4a

nehagup requested review from slayerjain and gouravkrosx May 18, 2024 11:45

Merge branch 'main' into fix-race-cond

78d1c9a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: race condition in keploy record #1867

fix: race condition in keploy record #1867

dxtym commented May 7, 2024

github-actions bot commented May 7, 2024 •

edited

github-actions bot left a comment

dxtym commented May 7, 2024

dxtym commented May 7, 2024

slayerjain left a comment

slayerjain May 9, 2024

slayerjain May 9, 2024

slayerjain May 9, 2024

gouravkrosx May 10, 2024

dxtym May 10, 2024

slayerjain May 10, 2024

dxtym May 12, 2024 •

edited

fix: race condition in keploy record #1867

Are you sure you want to change the base?

fix: race condition in keploy record #1867

Conversation

dxtym commented May 7, 2024

Related Issue

Describe the changes you've made

Type of change

Please let us know if any test cases are added

Describe if there is any unusual behaviour of your code(Write NA if there isn't)

Checklist:

Screenshots (if any)

github-actions bot commented May 7, 2024 • edited

github-actions bot left a comment

Choose a reason for hiding this comment

dxtym commented May 7, 2024

dxtym commented May 7, 2024

slayerjain left a comment

Choose a reason for hiding this comment

slayerjain May 9, 2024

Choose a reason for hiding this comment

slayerjain May 9, 2024

Choose a reason for hiding this comment

slayerjain May 9, 2024

Choose a reason for hiding this comment

gouravkrosx May 10, 2024

Choose a reason for hiding this comment

dxtym May 10, 2024

Choose a reason for hiding this comment

slayerjain May 10, 2024

Choose a reason for hiding this comment

dxtym May 12, 2024 • edited

Choose a reason for hiding this comment

Describe if there is any unusual behaviour of your code(Write `NA` if there isn't)

github-actions bot commented May 7, 2024 •

edited

dxtym May 12, 2024 •

edited