Skip to content

Add a check for when the command is canceled by the program on Window… #29538

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

Merged
merged 2 commits into from
Mar 2, 2024

Conversation

charles7668
Copy link
Contributor

Close #29509

Windows, unlike Linux, does not have signal-specified exit codes. Therefore, we should add a Windows-specific check for Windows. If we don't do this, the logs will always show a failed status, even though the command actually works correctly.

If you check the Go source code in exec_windows.go, you will see that it always returns exit code 1.
image

The exit code 1 does not exclusively signify a SIGNAL KILL; it can indicate any issue that occurs when a program fails.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 2, 2024
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 2, 2024

Could the code be simplified to this?

if ctx.Err() != nil {
    return ctx.Err()
}
return err

I think it should cover all cases.

  • If the command is killed by context, then use context error
  • Otherwise use command error

(I haven't carefully thought about this problem at the moment, just a brief idea)

@charles7668
Copy link
Contributor Author

Could the code be simplified to this?

if ctx.Err() != nil {
    return ctx.Err()
}
return err

I think it should cover all cases.

* If the command is killed by context, then use context error

* Otherwise use command error

(I haven't carefully thought about this problem at the moment, just a brief idea)

this is hard to test , considering the context may be canceled anywhere by the parent, I can't be sure that the error always returns before the context is canceled. If not, the real error will be ignored.

@charles7668 charles7668 force-pushed the FixCommandKillCheckOnWindows branch from a34310f to 078cfad Compare March 2, 2024 12:20
@wxiaoguang
Copy link
Contributor

Hmm, I think it's worth to do the change. According to the "Context" document:

	// If Done is not yet closed, Err returns nil.
	// If Done is closed, Err returns a non-nil error explaining why:
	// Canceled if the context was canceled
	// or DeadlineExceeded if the context's deadline passed.
	// After Err returns a non-nil error, successive calls to Err return the same error.
	Err() error

I think using ctx.Err() correctly would make code more correct. If you have worries about it .... I could do such "refactoring", if I am wrong I will follow it and propose related fixes. 😁

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

The check is pretty strict, so it could be good enough.

@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 2, 2024
@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 2, 2024
@yardenshoham yardenshoham added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 2, 2024
@lunny lunny enabled auto-merge (squash) March 2, 2024 13:21
@lunny lunny merged commit 423372d into go-gitea:main Mar 2, 2024
@GiteaBot GiteaBot added this to the 1.22.0 milestone Mar 2, 2024
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 2, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 2, 2024
* upstream/main:
  Fix a bug returning 404 when display a single tag with no release (go-gitea#29466)
  Add a check for when the command is canceled by the program on Window… (go-gitea#29538)
  Fix incorrect redirection when creating a PR fails (go-gitea#29537)
  Fix incorrect subpath in links (go-gitea#29535)
  Fix issue link does not support quotes (go-gitea#29484) (go-gitea#29487)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 10, 2024
@charles7668 charles7668 deleted the FixCommandKillCheckOnWindows branch April 10, 2024 01:21
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to open checker on windows
5 participants