Change timeout semantics, support altering timeout per-eval #69
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The goal is to alter the behavior of Sandbox so that the execution
timeout is measured from the time of calling #eval, instead of the time
of creating the sandbox.
To achieve this, instead of placing a timeout struct on the heap and
passing that as the user data pointer, we allocate a mutable Crystal
class with a start point field that we can update prior to engaging the
VM.
I also had to do two monkeypatches to the Context methods that serve as
the edge of Crystal code before calling into LibDuk. This may not be the
proper way to do this, but it is an awkward situation, and Sandbox is
the only descendant of Context, so I'm not sure what the intention is..
The next commit will complete the wiring of this new object.
---
As a bonus, this adds a ctor that takes a time span instead of just an
int, so you can expressively decalre the timeout as such:
Duktape::Sandbox.new(500.milliseoncds)
instead of having to check what units are expected.
It is not well known, but Crystal allows you to export symbols that can be called by foreign code. As such, this replaces the C procedure definted in the duk_config.h header with a Crystal native `fun` definition. In this `fun`, we cast down the user data pointer - a pointer to our TimeoutData struct for the executing Sandbox instance - and the timeout measurement is made. This could have been done by altering the C program as well. But I figured that moving this to native Crystal code would be preferred.
Add a forwarding method to Runtime
A little awkward because of backwards compat. Perhaps it should just be dropped?
|
Hi @z64! This is amazing! Thank you so much for the contribution! I'd love to merge this feature into Thanks! |
|
@jessedoyle You're welcome! 😸 It is inconsequential for us, but the main issues with merging it into duktape publicly are:
Once those are resolved I think this changeset should be in good shape. |
|
This PR looks great! I was trying to implement this myself as my use case is rather similar, and thankfully stumbled across this PR before I got anywhere. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi Jesse! I have an interesting PR for you.
We have a particular use case wherein we do something like the following:
Sandboxinstance (viaRuntime)This is a latency sensitive context, so we want to apply an execution timeout. However a few things are at odds with the current implementation:
Sandbox.new, noteval!So we were left with potentially having to create a new sandbox every time in order for the timeout to function well, but at the cost of awful latency.
I did some digging and found it easy enough to alter duktape to fit our use-case, which is namely allowing us to set a timeout that is measured per-
eval, and allows us to take as much time we need loading our prelude into ourRuntimesingleton.All that said, I leave these commits for your consideration. You will find my notes in code & commits. Please feel free to alter this branch as you see fit, we are using an internal fork for now, and I don't have much time to participate in code review.. but I'm happy to answer any questions or maybe I did something terrible 😸
Hope all is well ❤️