Skip to content
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

Avoid writing to shared memory in go-routine. #4360

Merged
merged 1 commit into from Mar 1, 2022

Conversation

absoludity
Copy link
Contributor

@absoludity absoludity commented Mar 1, 2022

Signed-off-by: Michael Nelson minelson@vmware.com

Description of the change

This change is a refactor only. It updates to:

  1. Move the functionality of the go-routine into a typed function returning both the available pkg summary and error,
  2. Update so that the go-routine communicates via the channel, sending the available pkg summary and error, rather than writing to shared memory.

As a result, we no longer need to worry about nil entries in a separate slice etc.

Note:

  • I've also include the index in the channel message, just so the current order is maintained.
  • I've added a test for fetching a non-initial page of results (as when I refactored I knew I'd broken it at first, but tests passed).

Benefits

Main benefit is that we're preferring communication via channels (and using that channel for syncing results) rather than using shared memory with an extra wait group. Not a big deal, but good to have an example to follow in other parts of the code, imo.

Possible drawbacks

Applicable issues

  • fixes #

Additional information

I don't mind if you prefer not to change this.

Up next: I have a situation I want to investigate, where I've added the TCE repo, I can see the packages (and meta) in the cluster, but Kubeapps shows an empty catalog (both on main and in this branch). Probably something I'm doing wrong, but I'll investigate first thing tomorrow.

@absoludity absoludity added this to In progress in Kubeapps via automation Mar 1, 2022
@absoludity absoludity moved this from In progress to Waiting For Review in Kubeapps Mar 1, 2022
Copy link
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! Yep, a much better approach for avoiding dealing with nil values. Thanks for improving it!

Base automatically changed from 4328-carvel-no-repos-error to main March 1, 2022 23:04
Signed-off-by: Michael Nelson <minelson@vmware.com>
@absoludity absoludity force-pushed the 3428-carvel-no-repos-error-2 branch from 6483385 to 3aee2f3 Compare March 1, 2022 23:05
@absoludity absoludity merged commit f7079cf into main Mar 1, 2022
@absoludity absoludity deleted the 3428-carvel-no-repos-error-2 branch March 1, 2022 23:47
Kubeapps automation moved this from Waiting For Review to Done Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Kubeapps
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants