Don't show context cancelled errors in attribute reader

[zeripath]

Fix #18997

Signed-off-by: Andrew Thornton art27@cantab.net

[lunny]

But we may need to log them?

[zeripath]

We don't need to log these.

The program being ended because the context has been cancelled is a normal thing and it should not be logged.

Similarly the "signal: killed" error will only occur due to the process being killed. This will happen due to context cancellation.

These are not errors and should not be logged.

[wxiaoguang]

Why c.ctx.Err() is err means context is being cancelled, but not using Error() string matches "context canceled"?

The other expression below is using err.Error() matches "signal: killed" to mean the process was killed .....

[zeripath]

What's your question here?

We can ignore both of these as they're expected errors.

[wxiaoguang]

What's your question here?

We can ignore both of these as they're expected errors.

I was asking about the meaning about c.ctx.Err() != err. I still have difficulty to understand whether it is correct in all cases.

c.ctx.Err() != err is a very rare expression for this scenario, most developers just seldom see or use it.

Question 1: Is it really correct?

For example:

• case 1: c.ctx.Err() = "context canceled" and err = "other error"?
• case 2: c.ctx.Err() = "other error" and err = "context canceled"?
• case 3: c.ctx.Err() = "the-error" and err = "the-error"?

Are they all correct for the expression c.ctx.Err() != err?

Even it's correct, it's not obvious, it's hard to imagine that future maintainers can know why it works.

Question 2: The error detection

The other expression is err.Error() != "signal: killed", so could the new expression be err.Error() != "context canceled" here? If not, what's the reason.

[wxiaoguang]

ps: to clarify .... the questions above are not a blocker. Maybe only I have the difficulty to understand the logic ....

If someone can confirm it's correct, just give the approval (and I would really appreciate if someone could help explain the logic and questions .... 🙏)

[zeripath]

Yes they are all correct.

Read the docs for context err. Different errs can be returned from the context err. If a context is cancelled due to its parent being cancelled it will have the same error.

It doesn't matter what caused the context to be cancelled it is cancelled.

The signal killed error also needs to be ignored due to cmd.Wait sometimes returning that when the exec is killed by the context being out of date.

[wxiaoguang]

If it is the case, I think we should patch RunContext

gitea/modules/git/command.go

Lines 182 to 186 in 98f5408

	if err := cmd.Wait(); err != nil && ctx.Err() != context.DeadlineExceeded {
	return err
	}
	return ctx.Err()

	if err := cmd.Wait(); err != nil && ctx.Err() != context.DeadlineExceeded { 
		return err 
	} 
	if ctx.Err() == context.Canceled {
		// if the command (context) is canceled by our code, we shouldn't treat such result as an error
		return nil
	}
	return ctx.Err()

---

Or

We just check err != context.Canceled （errors.Is) in this PR (instead of ctx.Err() != err)

---

In this PR, if we use ctx.Err()!=err, then DeadlineExceeded would never be reported as error anymore (case 3 in the previous comment), but it seems to be better to report it, because the process might have been hanging for a while, there could be a bug or something wrong.

[wxiaoguang]

IMO err != context.Canceled could be more clear and correct, and we should report DeadlineExceeded as error.

If you are sure that c.ctx.Err() != err is by purpose, the comment could be updated to We should not pass up error due to the context being cancelled or it exceeds deadline