Skip to content

feat: add method CancelWithError()#589

Open
jcmfernandes wants to merge 18 commits into
google:masterfrom
bckground:cancel-thread-with-error
Open

feat: add method CancelWithError()#589
jcmfernandes wants to merge 18 commits into
google:masterfrom
bckground:cancel-thread-with-error

Conversation

@jcmfernandes
Copy link
Copy Markdown

I'm interested in knowing if the execution was cancelled, and I don't want to rely on string matching the error message to know it.

@google-cla
Copy link
Copy Markdown

google-cla Bot commented Apr 28, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Copy Markdown
Collaborator

@adonovan adonovan left a comment

Choose a reason for hiding this comment

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

Thanks for this PR.

It is crucial that cancellation preserve the backtrace of the Starlark call stack so that the user can tell what function was interrupted by cancellation. The existing code in Call will cause the leaf error (cancellation) to be wrapped by EvalError, if it's not already an EvalError, so I think it should work correctly. But can you add a test (or modify your current test) so that a stack of calls f->g->h is cancelled while h is running (e.g. counting to infinity), and assert that the resulting error has both (a) a call stack showing f, g, h and responds to errors.Is(err, Cancelled)?

Comment thread starlark/eval.go Outdated
Comment thread starlark/eval.go Outdated
Comment thread starlark/eval.go Outdated
Comment thread starlark/eval.go Outdated
Comment thread starlark/interp.go Outdated
Comment thread starlark/eval.go Outdated
Comment thread starlark/eval_test.go Outdated
Comment thread starlark/eval.go Outdated
Comment thread starlark/eval_test.go Outdated
Comment thread starlark/interp.go Outdated
Comment thread starlark/eval.go Outdated
@jcmfernandes
Copy link
Copy Markdown
Author

Hey @adonovan, thanks for the review. I addressed your feedback. Let me know what you think.

Comment thread starlark/eval_test.go
Comment thread starlark/eval_test.go
Comment thread starlark/eval_test.go Outdated
`
cancelErr := errors.New("nope")
go func() {
for !ready.Load() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is not how to wait for an event; use a semaphore (sync.WaitGroup) or just a channel.

But why not just have the ready function cancel its own Thread? The concurrency of polling is not what's being tested here. The Starlark program then reduces to:

def f(): g()
def g(): h()
def h(): cancel()

and the cancel builtin function becomes:

"cancel": starlark.NewBuiltin(...) {
		thread.CancelWithError(cancelErr)
		return starlark.None, nil
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I see value in testing internally (sync?) and externally (async?) triggered cancellations, but I'm happy to change it.

Comment thread starlark/eval_test.go
@jcmfernandes jcmfernandes force-pushed the cancel-thread-with-error branch from 80929a3 to 22391d3 Compare May 1, 2025 13:18
@jcmfernandes jcmfernandes force-pushed the cancel-thread-with-error branch from 3ab82aa to aea70b0 Compare November 1, 2025 23:22
Copy link
Copy Markdown
Collaborator

@adonovan adonovan left a comment

Choose a reason for hiding this comment

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

Please discuss bugs and features in the issue tracker before sending code changes. Thanks.

@jcmfernandes
Copy link
Copy Markdown
Author

Please discuss bugs and features in the issue tracker before sending code changes. Thanks.

Can you please clarify what you expect at this point?

@adonovan
Copy link
Copy Markdown
Collaborator

adonovan commented Nov 5, 2025

Please discuss bugs and features in the issue tracker before sending code changes. Thanks.
Can you please clarify what you expect at this point?

Sure: I expect that before non-trivial code changes, we open an issue in this GitHub repo's issue tracker, describe the problem and the proposed solution, flag any potential difficulties, and come to a consensus on whether and how to fix it. This avoids wasting time in code review, and forms an easily searchable record of what problems exist or have been fixed.

@jcmfernandes
Copy link
Copy Markdown
Author

Please discuss bugs and features in the issue tracker before sending code changes. Thanks.
Can you please clarify what you expect at this point?

Sure: I expect that before non-trivial code changes, we open an issue in this GitHub repo's issue tracker, describe the problem and the proposed solution, flag any potential difficulties, and come to a consensus on whether and how to fix it. This avoids wasting time in code review, and forms an easily searchable record of what problems exist or have been fixed.

Will do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants