Adding spec for timeout in runtime initialize

ray-delossantos commented 8 years ago

jessedoyle commented 8 years ago

@raydf - Awesome! Thanks for the PR!

Is it okay if I close #14, because this PR effectively contains the same code?

ray-delossantos commented 8 years ago

Yes, please, i had a confusion with the github merging system.

jessedoyle commented 8 years ago

Also I'll make a few comments:

I'm certainly okay with the concept of this PR and adding a timeout to the Runtime class.

Anyone who uses the timeout mechanism should be aware of a few things:

Due to the current implementation, an initial timestamp is taken when the Duktape::Sandbox instance is created. Not when code is evaluated on it.
This means that anyone creating a Duktape::Runtime instance with a timeout and a lot of JS initializer code may trigger a timeout before calling any of the Duktape::Runtime API methods.
This initial timestamp is also not reset after any of the evaluation calls are made. Effectively, this means that the timeout value given is an absolute maximum for the practical lifespan of a Runtime or Sandbox instance.

For example:

sbx = Duktape::Sandbox.new 500 # 500ms
sbx.eval!("print('Hello, World!');") # => Hello, World!
sleep 1
sbx.eval!("print('Hello, World!');") # => RangeError

I believe this to be the correct behaviour, what do you think?

jessedoyle commented 8 years ago

Its also worthwhile to read about the limitations to the Duktape bytecode execution timeout implementation.

ray-delossantos commented 8 years ago

I believe this is the correct behavior. The timeout is important for shutting down infinite loops from untrusted javascript code and should not be used for state machines or long workflows. To anyone using this library, i'll recommend to just use it for small runtime execution, never for real coding like nodejs or any other lua framework.

jessedoyle commented 8 years ago

Okay great, we both agree! The code looks fine to me so I'll merge it.

I'm going to set the version in master branch to 0.7.1.pre and hold off on releasing a new stable version likely for a few weeks.

If you need to use this code in your own projects, just tell shards to use the master branch:

dependencies:
  duktape:
    github: jessedoyle/duktape.cr
    branch: master

Thanks!

jessedoyle / duktape.cr

Adding spec for timeout in runtime initialize #15