Closed tahpot closed 9 years ago
Bump?
Sorry for not answering earlier.
It definitely should be possible, but haven't tried yet. I wonder whether V8Js should provide a config option, enabling this behaviour. So the extension user can decide whether the exception should be escalated to JS side, and then back to PHP side (if JS doesn't catch) ... or directly to PHP
Stepping forward to implement this I stumbled over the fact, that JS execution continues even after the PHP exception was thrown.
<?php
class Foo {
function throwException() {
throw new \Exception("Test-Exception");
}
}
$v8 = new V8Js();
$v8->foo = new \Foo();
$JS = <<< EOT
try {
PHP.foo.throwException();
// the exception should abort further executing, hence
// the print must not pop up
print("after throwException\\n");
} catch(e) {
// JS should not catch in default mode
print("JS caught exception");
}
EOT;
try {
$v8->executeString($JS);
} catch (Exception $e) {
var_dump($e->getMessage());
var_dump($e->getFile());
var_dump($e->getLine());
}
?>
The text "after throwException" currently appears, so probably not expected :-)
Apparently Javascript execution continues, but any PHP-method won't be called. Strange.. I verified with an older version of V8Js that it's been "always" like that.
Oh wow, you're right.
I've been having some weirdness going on which I hadn't investigated fully, but this would be the cause.
Hmm, tests/exception_propagation_2.phpt
even relies on this. I'm currently a bit unsure on how to fix this...
executeString
exits it checks for pending exceptions from previous calls and throws these; however after the code itself was executedSo if we now make sure to immediately stop JS execution if a PHP exception is thrown, then ...
class Foo {
private $v8 = NULL;
public function __construct()
{
$this->v8 = new V8Js(null, array(), array(), false);
$this->v8->foo = $this;
$this->v8->executeString('fooobar', 'throw_0');
var_dump($this->v8->getPendingException());
$this->v8->executeString('try { PHP.foo.bar(); } catch (e) { print(e + " caught!\n"); }', 'trycatch1');
$this->v8->executeString('try { PHP.foo.bar(); } catch (e) { print(e + " caught!\n"); }', 'trycatch2');
}
public function bar()
{
echo "To Bar!\n";
$this->v8->executeString('throw new Error();', 'throw_1');
}
}
try {
$foo = new Foo();
... fails.
executeString
call causes an JS error, which is stored as a pending exceptiongetPendingException
call displays it, however doesn't clear itexecuteString
call provides a try/catch for and Error thrown and calls bar
before exitingbar
function calls executeString
to throw the Error; and executeString
will exit now, hence tests for the pending exception, notices it and throws a PHP exception + stops execution thereexecuteString
is now not executed anymoreBesides this test also actually tests that the third executeString
in __construct
isn't called, since a PHP exception is now pending.
This leads me to some options
I think options 2 to 4 might feel a bit magic to the user; I tend to pick option 1 ... what do you think?
This will get particularly funny if we throw PHP exceptions to JS; in the example above the JS-catch wouldn't get the expected Error thrown by bar, but the pending exception from a prior executeString call ... ... which again would be kindof magic to the user; hence should we only throw any other PHP exceptions to JS? What about other V8Js exceptions? e.g. V8 calls PHP code, that calls V8 code in turn, that causes some kind of script exception or timeout? Shall the JS code be able to see other V8 instances errors then?
I tend to accept this behaviour, if the implementor enables PHP->JS exception passing, any exception is passed, even unhandled pending ones
enough stuff for discussion, what do you think? @tahpot, @rosmo and others?
and another question: how to propagate the exception at best?
Simply convert any exception to an Error object, run getMessage
on the Exception object and simply propagate the message? Or better prepend some text like "PHP Exception:"?
Export the complete exception object to JS, so further method calls on it are possible?
And if JS doesn't catch the exception, what then? Throw simply ScriptException? Or a ScriptException and attach the original exception as previous exception?
and another question: how to propagate the exception at best?
I think it's best to replicate the information available in the PHP exception as much as possible.
Unsure if this is possible, but something like follows would be nice:
As per your propagation questions, I'm still trying to get my head around it...
the problem occurs if immediate exception propagation is disabled
What is the use case for disabling exception propagation? That's unclear to me, which makes it difficult to consider the most appropriate solution.
What about other V8Js exceptions? e.g. V8 calls PHP code, that calls V8 code in turn, that causes some kind of script exception or timeout? Shall the JS code be able to see other V8 instances errors then?
I've run into this quite a bit and vote yes. Ideally you want the most accurate as possible history of exceptions otherwise debugging becomes very painful and it limits your ability to properly handle exceptions.
@tahpot I don't really know what disabled exception reporting is really good for, after all this behaviour can easily be achieved by wrapping the executeString
method calls with try/catch blocks (after all you need error handling no matter what). It just was around when I took over maintainership.
@rosmo maybe you can tell why it was implemented?
@stesie, I can't really recall... probably for some sort of compatibility stuff. Obviously it's correct to stop the JS execution on exception, but I would like this to be configurable (by default on is good) to keep things for breaking on our end.
@rosmo I'm not sure if it's a good idea to keep all stuff, that was around once, forever.
Currently V8Js has no stuff marked as deprecated, and I agree that we need to have backwards compatibility for some time. But not forever, especially if it was marked as deprecated for a while.
Stuff that comes to my mind immediately
executeString
method -> preferred way is using setTimeLimit
and setMemoryLimit
callsexecuteString
method, after all they're stored globally anyways__construct
(preferred way: setting properties on V8Js object: especially since behaviour currently differs slightly)I consider publishing V8Js in 0.3.x version with PHP 7 compatibility only and all that stuff removed. And leaving 0.2.x around with PHP 5.x support and those features, but marked deprecated
@stesie, Yeah, that seems like a decent plan to me.
I regularly have JS code executing in V8 which calls PHP code. If that PHP code raises an exception, I want to catch the exception in Javascript.
Is that possible at all?
Here's some example Javascript: