Time/memory cannot be limitted with V8Function calls

[stesie]

Suggested by @christiaan on #127 , quote:

--- start --- I've been giving it some more tought.. Besides fixing this behaviour would it be possible to have something like this? To make the whole V8Function more usable.

<?php
$sandbox = new V8Js();

$cb = $sandbox->executeString('(function(a, b) { return a + b; });');

$cbArgs = array(2, 3);
$sandbox->executeFunction($cb, $cbArgs, $time_limit, $memory_limit); // returns 5
// Or
$cb->execute($cbArgs, $time_limit, $memory_limit); // returns 5

Another option could also be to make it impossible to invoke the V8Function from outside of a V8 context then I'll just work around it. Which seems to be the supported way of doing it anyway. --- eof ---

my answer there:

Concerning your most recent note, I think what you're trying to achieve is limitting time and memory consumption when calling V8Functions, right? I stumbled across the fact that that's not possible while fixing the bug ... never noticed before, but never missed it so far also, but I see & agree it's needed.

So I'm not sure if a executeFunction call is pretty to be honest. I like that it's possible to simply call the functions "the PHP way" (it has that call_user_func smell) ... and your second suggestion either doesn't work with functions having unlimited number of arguments or wouldn't be backwards compatible if you don't make them optional.

I was thinking of either

• passing time/memory limits to V8Js::__construct
• having time/memory limit setters on the V8Js object
• having php.ini entries

Regarding the time limit we could also distinct between "single call maximum runtime" and "total maximum runtime". I.e. tell JS to never execute for more than 50ms, even if it's 5000 calls from PHP to JS.

What do you think?

[christiaan]

Having the limits on the V8Js would also free up the rest of the API a bit. So IMHO this would be a good addition also increasing the readability of the overall API. Eg:

$v8js->executeString($script, $identifier, $flags, 500, 1024);
// VS
$v8js->setTimeLimit(500);
$v8js->setMemoryLimit(1024);
$v8js->executeString($script, $identifier);

The same goes for the executeScript function of course.

My preference would be the setters as those are most explicit and also clearly communicates the fact that the limits are optional.

[pinepain]

What about extensions time and memory limits?

[stesie]

@christiaan I agree and I also like the setters best. A problem that meanwhile came to my mind is that with nested executeString/executeScript calls you can apply different maximum execution times on the inner calls, which would look a bit awkward with the setters.

class Foo {
  public function bar() {
    $this->v8js->setTimeLimit(500);
    $this->v8js->executeString('... do something loooongish'); // 500 ms here
  }    
}

$v8js->foo = new Foo($v8js);
$v8js->setTimeLimit(100);
$v8js->executeString(' ... eventually call PHP.foo.bar(); ... ');  // 100 ms here

@pinepain good point, but I don't think it's possible to implement. Actually even the time & memory limits on the "normal" script stuff isn't a feature v8 provides us, but v8js sets up a watchdog thread that checks time & memory limits in 10ms intervals and forcefully terminates execution if the limits are exceeded.

Since the extensions are an internal thing of v8 it isn't possible to terminate script execution explicitly there. They can just be limitted indirectly with limits on normal script execution. But it's not a good idea to load "to be sandboxed" code into extensions anyway as they are not part of php's "shared nothing" request cycle ... so I think not an immediate problem (like time limit doesn't immediately terminate execution if you're exceeding it within php-code called from js).

cheers ~stesie

[pinepain]

It's not a problem at all that limits are not milliseconds accurate.

Is it correct that time and memory limits applied to script execution process, so extensions and script itself (incl requires) are counted together. I had a brief look of how's that done internally but it requires better knowledge in v8 internals than mine.

Thanks!

[stesie]

@pinepain there are multiple dimensions to that topic.

Generally all user code (executeString et al), extensions and the require'd code share the same isolate (but are seperated by different contexts within this isolate) ... and memory limits are checked against the total heap size of every isolate. Hence yes, they are counted together.

Time limits are currently applied on a call base, i.e. every time you call executeString no more time than the given number of milliseconds may pass, otherwise v8 is told to stop execution. So yes, if the user script calls either require'd or extension code and time limit occurs within there, execution is still stopped.

However there is no "global" limit with regard to the extension, so fun stuff happens if you register an extension that already takes a lot of memory and in turn call executeString with a memory limit lower than what's already taken by the extension :) like so:

$JS = <<< EOT
var text = "abcdefghijklmnopqrstuvwyxz0123456789";
var memory = "";
for (var i = 0; i < 1000000; ++i) {
    memory += text;
}
EOT;

V8Js::registerExtension('foo', $JS, [], true);

$v8 = new V8Js();

$JS = <<< EOT
for(var i = 0; i < 10; i ++) {
    print(i + '\\n');
    sleep(1);
}
EOT;

$v8->executeString($JS, 'basic.js', V8Js::FLAG_NONE, 0, 10000000);

Anyways, that's expected behaviour :)

... and I think there's no need to limit time & memory consumed while bootstrapping extensions. Even so it might be possible to implement, I don't think it makes much sense and wouldn't fit with the API outlined above

cheers ~stesie

[christiaan]

@stesie I think doing $v8->setTimeLimit(1000); should work similar to http://php.net/set_time_limit. So that it affects all javascript calls in the context.

So in your nested call where you set the limit to 500 it would just increase the limit of the context from 100 to 500. And as a consequence also affect the first call which was initiated with 100 ms.

Would that make sense?

[stesie]

@christiaan I like the idea, that setTimeLimit should behave similar to set_time_limit, that way you can even have pretty low time-limits and prolong the lifetime on every call from JS to PHP (i.e. even without nested calls from PHP to JS).

Like so:

class Foo {
  public function bar() {
    $this->v8js->setTimeLimit(100);  // PHP method does take a while + JS code may run loooongish, if the bar function is called regularly
    // do something ....
  }    
}

$v8js->foo = new Foo($v8js);
$v8js->setTimeLimit(100); // low limit for JS code
$v8js->executeString('
  for(var i = 0; i < 10000; i++) {
    PHP.foo.bar();
    // 100 ms here per iteration
   }
');

This could also be implemented side-by-side with the current time-limitting stuff, i.e. we can leave the executeString limits around and even keep the nesting behaviour. Just if you don't provide limits with executeString the setTimeLimit stuff is applied.

[christiaan]

I just learned something :smile:. I didn't know that php's set_time_limit worked like that.. resetting the current time that already passed and restarting the timer for the new limit.

If you can make it behave the same as set_time_limit that would be awesome and easy to understand for PHP developers looking at the code.

[stesie]

yes, that's a pretty unknown feature with set_time_limit :smile: Should be quite straight forward to implement, I'll have a look tonight or tomorrow.

[stesie]

If you're interested in trying my patch out, give the branch https://github.com/stesie/v8js/compare/issue-129 a try.

I've noticed instabilities regarding the time/memory limit checking thread which causes segfaults sporadically. The problem occurs if the heap size checking happens while v8 runs the garbage collection internally. V8Js is expected to lock the isolate when checking the heap size (which it currently doesn't do however); I need to try that a bit more, since you cannot just lock but need to interrupt execution temporarily etc.pp -- if done, I'll follow up with a pull request

Anyways, if you'd like to, feel free to give it a try

cheers ~stesie