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

Refactor git.Command.Run*, introduce RunWithContextString and RunWithContextBytes #19266

Merged
merged 10 commits into from
Mar 31, 2022

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Mar 30, 2022

This PR follows

Introduce RunWithContextString and RunWithContextBytes to help the refactoring. Add related unit tests. They keep the same behavior to save stderr into err.Error() as RunInXxx before.

Remove RunInDirTimeoutPipeline RunInDirTimeoutFullPipeline RunInDirTimeout RunInDirTimeoutEnv RunInDirPipeline RunInDirFullPipeline RunTimeout, RunInDirTimeoutEnvPipeline, RunInDirTimeoutEnvFullPipeline, RunInDirTimeoutEnvFullPipelineFunc.

Then remaining RunInDir RunInDirBytes RunInDirWithEnv can be easily refactored in next PR with a simple search & replace:

  • before: stdout, err := RunInDir(path)
  • next: stdout, _, err := RunWithContextString(&git.RunContext{Dir:path})

Other changes:

  1. When timeout <= 0, use default. Because timeout==0 is meaningless and could cause bugs. And now many functions becomes more simple, eg: GitGcRepos 9 lines to 1 line. Fsck 6 lines to 1 line.
  2. Only set defaultCommandExecutionTimeout when the option setting.Git.Timeout.Default > 0

Now this PR is +127 −126 with more unit tests

@wxiaoguang wxiaoguang added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Mar 30, 2022
@wxiaoguang wxiaoguang added this to the 1.17.0 milestone Mar 30, 2022
@wxiaoguang wxiaoguang added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Mar 30, 2022
modules/git/command.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 30, 2022
@wxiaoguang wxiaoguang requested a review from Gusted March 30, 2022 07:49
modules/git/command.go Outdated Show resolved Hide resolved
modules/git/command.go Outdated Show resolved Hide resolved
modules/git/command.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 30, 2022
@@ -86,6 +86,10 @@ func (pm *Manager) AddContext(parent context.Context, description string) (ctx c
// Most processes will not need to use the cancel function but there will be cases whereby you want to cancel the process but not immediately remove it from the
// process table.
func (pm *Manager) AddContextTimeout(parent context.Context, timeout time.Duration, description string) (ctx context.Context, cancel context.CancelFunc, finshed FinishedFunc) {
if timeout <= 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like there are some user-defined timeouts being passed to this function, I'm not sure that in the middle of normal operations a panic should occur from a process...

Reference:

ctx, _, finished := process.GetManager().AddContextTimeout(graceful.GetManager().HammerContext(), setting.MailService.SendmailTimeout, desc)

Copy link
Member

Choose a reason for hiding this comment

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

hm that would mean we either should fail on gitea init or put out a warn and ignore this setting

Copy link
Contributor Author

@wxiaoguang wxiaoguang Mar 31, 2022

Choose a reason for hiding this comment

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

@Gusted

When passing timeout<=0 into AddContextTimeout:

  • Before: then the context is cancelled immediately (a non-obvious bug, which may lead to more strange problems)
  • Now: a panic is reported as early as possible, when we catch the bug and can fix it.

From my experience, in a big and complex system, everything should be defined as a clear behavior, if there is something unexpected happening, the error should be reported in the first time. We should always expose mistakes, instead of hiding mistakes.

So I think this new mechanism is better and more stable, this refactoring is only done in 1.17 and we have enough time to catch more bugs in future.

And fortunately, the SendmailTimeout has a default value: SendmailTimeout: sec.Key("SENDMAIL_TIMEOUT").MustDuration(5 * time.Minute),, so there will be no problem now. As an exmple, if user set SendmailTimeout=-1 in old code, the user only sees some non-sense errors like context cancelled. Now, the user knows that the timeout is wrong. And when he reports the bug to gitea issues, the panic message is also very clear for maintainers.

Copy link
Member

Choose a reason for hiding this comment

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

Or we could ignore the timeout context if timeout <= 0 but not panic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, ignoring non-sense values in low-level frameworks is wrong. Many Go functions also panic if there is an invalid input.

Copy link
Contributor

Choose a reason for hiding this comment

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

And when he reports the bug to gitea issues, the panic message is also very clear for maintainers.

I think we should prevent such issues from being reported. I'm not sure at which level the context cancelled are being reported to the admin, but otherwise it might be their "first time" that they observe that their configuration is incorrect. I think we should have a little note in the 1.17 announcement that a negative timeout can lead to panics and errors and that 0 should be used instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Many Go functions also panic if there is an invalid input.

Many were written without being able to return errors, and given they don't want to break backwards compatibility, they instead panic.

Copy link
Contributor Author

@wxiaoguang wxiaoguang Mar 31, 2022

Choose a reason for hiding this comment

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

And when he reports the bug to gitea issues, the panic message is also very clear for maintainers.

I think we should prevent such issues from being reported. I'm not sure at which level the context cancelled are being reported to the admin, but otherwise it might be their "first time" that they observe that their configuration is incorrect. I think we should have a little note in the 1.17 announcement that a negative timeout can lead to panics and errors and that 0 should be used instead.

Why do we need to mention that? If users were using -1 or 0, how can their system work correctly? And 0 is also incorrect for a timeout, it should be handled by Gitea's setting module to set to a default timeout, this work can not be done automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to mention that? If users were using -1 or 0, how can their system work correctly?

I have no clue, but from what I've learned is that people can somehow survive with incorrect configurations without noticing. Anyhow, approving.

Copy link
Contributor Author

@wxiaoguang wxiaoguang Mar 31, 2022

Choose a reason for hiding this comment

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

To be clear:

If a user were using -1 or 0, and their system works correctly, after this PR, nothing changes. Because these values were processed to defaults correctly by Gitea already.

If a user were using -1 or 0 and their system didn't work correctly, before this PR they knew nothing, after this PR they know that there is something wrong. That's a improvement.

ps: I agree "people can somehow survive with incorrect configurations without noticing.", however, once they meet some errors and report "bugs", it makes maintainers frustrated ..... nobody can guess what's wrong on user side if mistakes are hidden. We really need a clear system 😊

@wxiaoguang wxiaoguang requested a review from Gusted March 31, 2022 08:07
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 31, 2022
@wxiaoguang
Copy link
Contributor Author

Let's merge and try. If the panic is too annoying, then let's locate the root case why such impossible case happens and find proper solutions. Personally speaking I have confidence about the stability.

@6543 6543 merged commit b877504 into go-gitea:main Mar 31, 2022
@wxiaoguang wxiaoguang deleted the refactor-git-cmd branch March 31, 2022 11:56
wxiaoguang added a commit that referenced this pull request Apr 1, 2022
Follows #19266, #8553, Close #18553, now there are only three `Run..(&RunOpts{})` functions.
 * before: `stdout, err := RunInDir(path)`
 * now: `stdout, _, err := RunStdString(&git.RunOpts{Dir:path})`
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 1, 2022
* giteaoffical/main:
  Fix broken of team create (go-gitea#19288)
  Remove `git.Command.Run` and `git.Command.RunInDir*` (go-gitea#19280)
  Performance improvement for add team user when org has more than 1000 repositories (go-gitea#19227)
  [skip ci] Updated translations via Crowdin
  Update JS dependencies (go-gitea#19281)
  Fix container download counter (go-gitea#19287)
  go.mod: update kevinburke/ssh_config to v1.2.0 (go-gitea#19286)
  Fix global packages enabled avaiable (go-gitea#19276)
  Add Goroutine stack inspector to admin/monitor (go-gitea#19207)
  Move checks for pulls before merge into own function (go-gitea#19271)
  Restore user autoregistration with email addresses (go-gitea#19261)
  Improve sync performance for pull-mirrors (go-gitea#19125)
  Refactor `git.Command.Run*`, introduce `RunWithContextString` and `RunWithContextBytes` (go-gitea#19266)
  Move reaction to models/issues/ (go-gitea#19264)
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants