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

Support for context Cancelation and Timeout in func executions #1108

Merged
merged 13 commits into from
Feb 10, 2023

Conversation

mathetake
Copy link
Member

@mathetake mathetake commented Feb 8, 2023

Closes #509 and #422. The latter is slightly different, but I believe almost all use cases will be covered by this since
in any way exact CPU usage measurement is impossible. Timeout and Cancelation can effectively be used by
users to limit the execution times approximately. That is exactly the same as the concept of "fuel" where fuel is an abstractive thing and no one can interpret how 1 fuel corresponds to exactly how much CPU cycles are used across multiple platforms.

Notably, this adds WithCloseOnContextDone option for RuntimeConfig as below:

// WithCloseOnContextDone ensures the executions of functions to be closed under one of the following circumstances:
//
// 	- context.Context passed to the Call method of api.Function is canceled during execution. (i.e. ctx by context.WithCancel)
// 	- context.Context passed to the Call method of api.Function reaches timeout during execution. (i.e. ctx by context.WithTimeout or context.WithDeadline)
// 	- Close or CloseWithExitCode of api.Module is explicitly called during execution.
//
// This is especially useful when one wants to run untrusted Wasm binaries since otherwise, any invocation of
// api.Function can potentially block the corresponding Goroutine forever. Moreover, it might block the
// entire underlying OS thread which runs the api.Function call. See "Why it's safe to execute runtime-generated
// machine codes against async Goroutine preemption" section in internal/engine/compiler/RATIONALE.md for detail.
//
// Note that this comes with a bit of extra cost when enabled. The reason is that internally this forces
// interpreter and compiler runtimes to insert the periodical checks on the conditions above. For that reason,
// this is disabled by default.
//
// See examples in context_done_example_test.go for the end-to-end demonstrations.
//
// When the invocations of api.Function are closed due to this, sys.ExitError is raised to the callers and
// the api.Module from which the functions are derived is made closed.
WithCloseOnContextDone(bool) RuntimeConfig

With this, the following code works like a charm:

infiniteLoop := moduleInstance.ExportedFunction("infinite_loop")

// Create the context.Context to be passed to the invocation of infinite_loop.
ctx, cancel := context.WithTimeout(context.Background(), time.Second)
defer cancel()

// Invoke the infinite loop with the timeout context.
_, err = infiniteLoop.Call(ctx)

// Timeout is correctly handled and triggers the termination of infinite loop.
fmt.Println(err)

The only use case I think this cannot cover is that relative fuel consumption - users might want to compare how much Wasm instructions are used among different module function invocations. It can be used, for example, to bill Wasm function owners depending on the consumed fuel * pre determined price per fuel. In that case "exact" CPU cycles are not necessarily needed. This use case is out-of-scope until anyone asks since the implementation will be much more complex and different than this context integration. In any case, that can be implemented later.

Signed-off-by: Takeshi Yoneda takeshi@tetrate.io

@codefromthecrypt
Copy link
Contributor

cc @pims!

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@mathetake mathetake marked this pull request as ready for review February 9, 2023 04:11
@pims
Copy link
Contributor

pims commented Feb 9, 2023

Gave this a try with the wasi-python example I had and it works well ⛽ !

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@mathetake mathetake requested a review from evacchi February 9, 2023 07:00
@mathetake
Copy link
Member Author

@evacchi I think it would be great if you could review 👍

Comment on lines +485 to +494
// Insert the exit code check on the loop header, which is the only necessary point in the function body
// to prevent infinite loop.
//
// Note that this is a little aggressive: this checks the exit code regardless the loop header is actually
// the loop. In other words, this checks even when no br/br_if/br_table instructions jumping to this loop
// exist. However, in reality, that shouldn't be an issue since such "noop" loop header will highly likely be
// optimized out by almost all guest language compilers which have the control flow optimization passes.
if c.ensureTermination {
c.emit(OperationBuiltinFunctionCheckExitCode{})
}
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the key

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@evacchi
Copy link
Contributor

evacchi commented Feb 9, 2023

Will do!

Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

Excellent job! so many have asked about this, it is great to have a chance prior to 1.0!

Main thing I wonder is if this should be called RuntimeConfig.WithCloseOnContextCancel because I believe at the end of the day, all these things end up as cancelation causes. In the very least it should be obvious by name that this is about context integration because the other ways (e.g. someone calling module.Close()` already exist w/o this feature.

happy for other ideas, too. Besides the name bike shedding I only had some questions about edge cases, specifically to know if someone wraps a context things will still work.

@codefromthecrypt
Copy link
Contributor

or RuntimeConfig.WithCloseOnCanceledContext ...

@codefromthecrypt
Copy link
Contributor

@ncruces thoughts on this one?

@codefromthecrypt
Copy link
Contributor

@daixiang0 @ItalyPaleAle @yaron2 so this means wasm middleware can stop based on incoming context. I'm not sure if it is routine to use context-cancelation in dapr, but we can integrate it once this feature is cut if so.

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@mathetake
Copy link
Member Author

I will go with WithCloseOnContextDone since it includes Timeout and Cancel and matches the meaning of Go's context.Context.Done

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@ncruces
Copy link
Collaborator

ncruces commented Feb 10, 2023

@codefromthecrypt I made a quick pass over the code, but not enough to make detailed comments.
The feature seems useful, well thought out, and implemented.

From my experience, I'm unlikely to use it with SQLite (outside of tests?) because, although calling exit(1) randomly (this seems equivalent) is not supposed to lead to data corruption, it is still a pretty heavy handed way of doing it with a trusted library, that might have other, gentler, ways to handle interrupts.

I'm likely to use it with dcraw. I'm already using contexts in that environment (web server), and cancelling seems appropriate and not nefarious. It mimics killing an exec.Cmd subprocess on cancellation, which is a nice ability to have.

@mathetake mathetake merged commit 92e0a48 into main Feb 10, 2023
@mathetake mathetake deleted the ensuretermination branch February 10, 2023 00:39
@mathetake
Copy link
Member Author

Thank you folks, enjoy! 🍻

@pims
Copy link
Contributor

pims commented Feb 10, 2023

Thank YOU @mathetake 🙇🏻

@yaron2
Copy link

yaron2 commented Feb 10, 2023

@daixiang0 @ItalyPaleAle @yaron2 so this means wasm middleware can stop based on incoming context. I'm not sure if it is routine to use context-cancelation in dapr, but we can integrate it once this feature is cut if so.

We're building context cancellations for most components now so this would certainly become useful.

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.

Consider integration of go's cancel context
6 participants