Use sync/atomic to keep tab on go routines #5961
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Dharmit Shah <shahdharmit@gmail.com> As a result of the code pushed in #5942, `odo logs --follow` would hang if user executed it without pushing the component in either dev or deploy modes. This was not a problem before that PR was merged. This was caused because below code would not get executed in business layer if user was using follow mode: ``` if !follow { doneChan <- struct{}{} } ``` The reason we didn't execute it for follow mode was that it prematurely exited and often didn't print the logs of all the containers. This PR uses `sync/atomic` to keep a tab on go routines started for `odo logs --follow` instead of using `sync.WaitGroup`, and checks if no go routines are running before it exits. This enables us to use `doneChan <- struct{}{}` in business layer irrespective of user running `odo logs` with or without follow mode. Why not use `wg.Wait()` as first statement instead in `case <-events.Done:`? This is because `wg.Wait()` is a blocking call and won't be called upon each time a go routine for `odo logs --follow` is done (`wg.Done()`). This leads to same problem as that fixed by PR 5942, which is that `odo logs --follow` won't exit even if the underlying odo component was deleted.
✅ Deploy Preview for odo-docusaurus-preview canceled.
|
Signed-off-by: Dharmit Shah <shahdharmit@gmail.com> `go vet` complains about direct assignment to an atomic value. Hence, an anonymous struct to fix the problem.
|
Kudos, SonarCloud Quality Gate passed!
|
dharmit
requested review from
valaparthvi and
rm3l
and removed request for
feloy
valaparthvi
approved these changes
Jul 21, 2022
openshift-ci
bot
added
the
lgtm
|
/approve and thanks for the fix! |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdrage The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
cdrage
pushed a commit
to cdrage/odo
that referenced
this pull request
Aug 31, 2022
* Use sync/atomic to keep tab on go routines Signed-off-by: Dharmit Shah <shahdharmit@gmail.com> As a result of the code pushed in redhat-developer#5942, `odo logs --follow` would hang if user executed it without pushing the component in either dev or deploy modes. This was not a problem before that PR was merged. This was caused because below code would not get executed in business layer if user was using follow mode: ``` if !follow { doneChan <- struct{}{} } ``` The reason we didn't execute it for follow mode was that it prematurely exited and often didn't print the logs of all the containers. This PR uses `sync/atomic` to keep a tab on go routines started for `odo logs --follow` instead of using `sync.WaitGroup`, and checks if no go routines are running before it exits. This enables us to use `doneChan <- struct{}{}` in business layer irrespective of user running `odo logs` with or without follow mode. Why not use `wg.Wait()` as first statement instead in `case <-events.Done:`? This is because `wg.Wait()` is a blocking call and won't be called upon each time a go routine for `odo logs --follow` is done (`wg.Done()`). This leads to same problem as that fixed by PR 5942, which is that `odo logs --follow` won't exit even if the underlying odo component was deleted. * Use a struct instead of int64 for atomic Signed-off-by: Dharmit Shah <shahdharmit@gmail.com> `go vet` complains about direct assignment to an atomic value. Hence, an anonymous struct to fix the problem.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Dharmit Shah shahdharmit@gmail.com
What type of PR is this:
/kind bug
What does this PR do / why we need it:
As a result of the code pushed in #5942,
odo logs --followwould hangif user executed it without pushing the component in either dev or
deploy modes. This was not a problem before that PR was merged.
This was caused because below code would not get executed in business
layer if user was using follow mode:
The reason we didn't execute it for follow mode was that it prematurely
exited and often didn't print the logs of all the containers.
This PR uses
sync/atomicto keep a tab on go routines started forodo logs --followinstead of usingsync.WaitGroup, and checks if nogo routines are running before it exits. This enables us to use
doneChan <- struct{}{}in business layer irrespective of user runningodo logswith or without follow mode.Why not use
wg.Wait()as first statement instead incase <-events.Done:? This is becausewg.Wait()is a blocking calland won't be called upon each time a go routine for
odo logs --followis done (
wg.Done()). This leads to same problem as that fixed by PR5942, which is that
odo logs --followwon't exit even if theunderlying odo component was deleted.
Which issue(s) this PR fixes:
Fixes part of #5872
PR acceptance criteria:
Unit test
Integration test
Documentation
How to test changes / Special notes to the reviewer: