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

fix RegisterFlagCompletionFunc concurrent map writes error #1423

Merged

Conversation

@silenceshell
Copy link
Contributor

@silenceshell silenceshell commented Jun 24, 2021

Some CI system(like istio) will call package of istioctl in parallel, if cobra use flagCompletionFunctions as a global var, will generate fatal error: concurrent map writes.
So it is better to store those functions to flagCompletionFunctions as a map of root command(thread-safe), instead of a global var.

ref #1320 (this PR will not fix it, as we only get thread-safe, but no lock for multi threads)

@CLAassistant
Copy link

@CLAassistant CLAassistant commented Jun 24, 2021

CLA assistant check
All committers have signed the CLA.

@silenceshell
Copy link
Contributor Author

@silenceshell silenceshell commented Jun 24, 2021

@jpmcb would you like to review this PR? Thanks a lot!

command.go Outdated Show resolved Hide resolved
Copy link
Contributor

@marckhouzam marckhouzam left a comment

Thanks @silenceshell, I think this is simple and safe improvement regarding the global map.

/cc @jpmcb

@silenceshell
Copy link
Contributor Author

@silenceshell silenceshell commented Jun 29, 2021

@marckhouzam Thanks for your review.
Will this PR be merged to the next release? #1388
I am working on adding completion support for istioctl, and look forward for the next release to support flag completion for istioctl, this will make the command more handy.

@umarcor umarcor mentioned this pull request Jun 29, 2021
@marckhouzam
Copy link
Contributor

@marckhouzam marckhouzam commented Jun 29, 2021

@marckhouzam Thanks for your review.
Will this PR be merged to the next release?

I hope so, but it's up to the maintainers.

I am working on adding completion support for istioctl, ...

Cool! I've just started using istioctl and noticed it was lacking full completion. Thanks for your work on that.

@jpmcb
jpmcb approved these changes Jun 30, 2021
Copy link
Collaborator

@jpmcb jpmcb left a comment

This is great! And thanks for bringing this back to the community. LGTM!

@jpmcb jpmcb merged commit 3c8a19e into spf13:master Jun 30, 2021
7 checks passed
7 checks passed
@github-actions
triage
Details
@github-actions
ubuntu | 1.14.x
Details
@github-actions
ubuntu | 1.15.x
Details
@github-actions
macOS | 1.14.x
Details
@github-actions
macOS | 1.15.x
Details
@github-actions
MINGW64
Details
license/cla Contributor License Agreement is signed.
Details
Luap99 added a commit to Luap99/cobra that referenced this pull request Jul 2, 2021
The flag completion functions should not be stored in the root cmd.
There is no requirement that the root cmd should be the same when
`RegisterFlagCompletionFunc` was called. Storing the flags there does
not work when you add the the flags to your cmd struct before you add the
cmd to the parent/root cmd. The flags can no longer be found in the rigth
place when the completion command is called and thus the flag completion
does not work.

Also spf13#1423 claims that this would be thread safe but we still have a map
which will fail when accessed concurrently. To truly fix this issue use a
RWMutex.

Fixes spf13#1437
Fixes spf13#1320

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Luap99 added a commit to Luap99/cobra that referenced this pull request Jul 2, 2021
The flag completion functions should not be stored in the root cmd.
There is no requirement that the root cmd should be the same when
`RegisterFlagCompletionFunc` was called. Storing the flags there does
not work when you add the the flags to your cmd struct before you add the
cmd to the parent/root cmd. The flags can no longer be found in the rigth
place when the completion command is called and thus the flag completion
does not work.

Also spf13#1423 claims that this would be thread safe but we still have a map
which will fail when accessed concurrently. To truly fix this issue use a
RWMutex.

Fixes spf13#1437
Fixes spf13#1320

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Luap99 added a commit to Luap99/cobra that referenced this pull request Jul 2, 2021
The flag completion functions should not be stored in the root cmd.
There is no requirement that the root cmd should be the same when
`RegisterFlagCompletionFunc` was called. Storing the flags there does
not work when you add the the flags to your cmd struct before you add the
cmd to the parent/root cmd. The flags can no longer be found in the rigth
place when the completion command is called and thus the flag completion
does not work.

Also spf13#1423 claims that this would be thread safe but we still have a map
which will fail when accessed concurrently. To truly fix this issue use a
RWMutex.

Fixes spf13#1437
Fixes spf13#1320

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Luap99 added a commit to Luap99/cobra that referenced this pull request Jul 2, 2021
The flag completion functions should not be stored in the root cmd.
There is no requirement that the root cmd should be the same when
`RegisterFlagCompletionFunc` was called. Storing the flags there does
not work when you add the the flags to your cmd struct before you add the
cmd to the parent/root cmd. The flags can no longer be found in the rigth
place when the completion command is called and thus the flag completion
does not work.

Also spf13#1423 claims that this would be thread safe but we still have a map
which will fail when accessed concurrently. To truly fix this issue use a
RWMutex.

Fixes spf13#1437
Fixes spf13#1320

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
jpmcb pushed a commit that referenced this pull request Jul 2, 2021
* Fix flag completion

The flag completion functions should not be stored in the root cmd.
There is no requirement that the root cmd should be the same when
`RegisterFlagCompletionFunc` was called. Storing the flags there does
not work when you add the the flags to your cmd struct before you add the
cmd to the parent/root cmd. The flags can no longer be found in the rigth
place when the completion command is called and thus the flag completion
does not work.

Also #1423 claims that this would be thread safe but we still have a map
which will fail when accessed concurrently. To truly fix this issue use a
RWMutex.

Fixes #1437
Fixes #1320

Signed-off-by: Paul Holzinger <pholzing@redhat.com>

* Fix trailing whitespaces in fish comp scripts

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants