-
-
Notifications
You must be signed in to change notification settings - Fork 376
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
fix: race condition in keploy record #1867
base: main
Are you sure you want to change the base?
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you and congratulations 🎉 for opening your very first pull request in keploy
Singed-off-by: Dilmurod Abdusamadov dilmurod.abdusamadov2004@gmail.com |
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dxtym , Thanks for the PR! I have left some comments and I think @gouravkrosx / @charankamarapu should give more context for a better solution.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something but not really sure if this defer could be called concurrently 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aah, so its the testCount probably..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <akashsingh2210670@gmail.com> * Path init Signed-off-by: Akash <akashsingh2210670@gmail.com> * Successfully found the expected and actual responses for failing tests Signed-off-by: Akash <akashsingh2210670@gmail.com> * Files are being updated but structs are not proper: func WriteTestcase doesnt match yaml file Signed-off-by: Akash <akashsingh2210670@gmail.com> * Updating testcase files successfully Signed-off-by: Akash <akashsingh2210670@gmail.com> * Added flags to collect test-set and test-cases from user Signed-off-by: Akash <akashsingh2210670@gmail.com> * Fixed lint errors Signed-off-by: Akash <akashsingh2210670@gmail.com> * Fixed lint errors Signed-off-by: Akash <akashsingh2210670@gmail.com> * Refactored PR for Keploy v2 and fixed merge conflicts Signed-off-by: Akash Singh <akashsingh2210670@gmail.com> * Normalise Test cases Signed-off-by: Akash Singh <akashsingh2210670@gmail.com> * Addressed comments and refactored changes Signed-off-by: Akash Singh <akashsingh2210670@gmail.com> * Fixed Lint Signed-off-by: Akash Singh <akashsingh2210670@gmail.com> * Fixed Lint Signed-off-by: Akash Singh <akashsingh2210670@gmail.com> * Refactoring and Adding Additional Flags Signed-off-by: Akash Singh <akashsingh2210670@gmail.com> * Added Normalise to replay service Signed-off-by: Akash Singh <akashsingh2210670@gmail.com> * Fixed lint Signed-off-by: Akash Singh <akashsingh2210670@gmail.com> --------- Signed-off-by: Akash <akashsingh2210670@gmail.com> Signed-off-by: Akash Singh <akashsingh2210670@gmail.com> Co-authored-by: Animesh Pathak <53110238+Sonichigo@users.noreply.github.com> * feat: add normalization Signed-off-by: charankamarapu <kamarapucharan@gmail.com> * fix: bugs Signed-off-by: charankamarapu <kamarapucharan@gmail.com> * fix: bugs Signed-off-by: charankamarapu <kamarapucharan@gmail.com> * fix: bugs Signed-off-by: charankamarapu <kamarapucharan@gmail.com> * fix: resolve comments Signed-off-by: charankamarapu <kamarapucharan@gmail.com> --------- Signed-off-by: Akash <akashsingh2210670@gmail.com> Signed-off-by: Akash Singh <akashsingh2210670@gmail.com> Signed-off-by: charankamarapu <kamarapucharan@gmail.com> Co-authored-by: Sky Singh <114267538+Akash-Singh04@users.noreply.github.com> Co-authored-by: Animesh Pathak <53110238+Sonichigo@users.noreply.github.com> Signed-off-by: dxtym <dilmurod.abdusamadov2004@gmail.com>
Signed-off-by: dxtym <dilmurod.abdusamadov2004@gmail.com>
Signed-off-by: Amogh Umesh <amoghumesh02@gmail.com> Signed-off-by: dxtym <dilmurod.abdusamadov2004@gmail.com>
Co-authored-by: Sarthak Shyngle <50234097+Sarthak160@users.noreply.github.com> Signed-off-by: dxtym <dilmurod.abdusamadov2004@gmail.com>
* chore: add docker ip log and remove a switch Signed-off-by: Shubham Jain <shubhamkjain@outlook.com> * fix: check container ip before starting tests Signed-off-by: Shubham Jain <shubhamkjain@outlook.com> --------- Signed-off-by: Shubham Jain <shubhamkjain@outlook.com> Signed-off-by: dxtym <dilmurod.abdusamadov2004@gmail.com>
ougoung -> outgoing Signed-off-by: Priyansh Agrawal <agrawal.priyansh@yahoo.in> Co-authored-by: Sarthak Shyngle <50234097+Sarthak160@users.noreply.github.com> Signed-off-by: dxtym <dilmurod.abdusamadov2004@gmail.com>
Signed-off-by: dxtym <dilmurod.abdusamadov2004@gmail.com>
Signed-off-by: dxtym <dilmurod.abdusamadov2004@gmail.com>
Signed-off-by: dxtym <dilmurod.abdusamadov2004@gmail.com>
* fix: folder permissions Signed-off-by: charankamarapu <kamarapucharan@gmail.com> * fix: folder permissions Signed-off-by: charankamarapu <kamarapucharan@gmail.com> * fix: unset umask Signed-off-by: charankamarapu <kamarapucharan@gmail.com> * doc: add comment Signed-off-by: charankamarapu <kamarapucharan@gmail.com> * doc: add comment Signed-off-by: charankamarapu <kamarapucharan@gmail.com> --------- Signed-off-by: charankamarapu <kamarapucharan@gmail.com> Signed-off-by: dxtym <dilmurod.abdusamadov2004@gmail.com>
Signed-off-by: dxtym <dilmurod.abdusamadov2004@gmail.com>
@gouravkrosx please help close this |
I would greatly appreciate support 🙏 |
@dxtym, Sorry for the delay, will try to review this PR before EOD. |
Related Issue
testCount
while exiting keploy recordCloses: #1851
Describe the changes you've made
Put
sync.Mutex
locks over the sharing value so that concurrent activities don't cause race condition. Addsync.waitGroup
to control the flow of actions between the main and sub gorountines.Type of change
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:
Screenshots (if any)