Oh now I do remember the whole thing of why we return undefined. Good thing I added a test case :)
And yes, that's a stale TODO. Will remove it. On Tuesday, February 16, 2016 at 9:42:53 AM UTC+1, Jakob Kummerow wrote: > > Looks a lot like a stale TODO. Comments on the bug sound like we went > through roughly the same sequence of plans, insights, and backtracking as > you just did. > > On Tue, Feb 16, 2016 at 8:54 AM, Alex Kodat <[email protected] > <javascript:>> wrote: > >> Well, I feel like an idiot. Tried making the change and promptly blew out >> in test-thread-termination/ErrorObjectAfterTermination. The reason is that >> the native Error functions can't run if TerminateExecution has been called. >> So the "normal" case where the Exception::Error functions won't return a >> Local<Object> is when they return an Undefined because of >> TerminateExecution. I initially thought that the fact that this exposes the >> fact that the Exception::Error functions use native JS is a bit groty but >> came to the realization that this is reasonably good behavior. If you want >> to Throw a new Exception in C++ code but are TerminateExecution has been >> called, you get an Undefined back and if you just Throw it, the termination >> continues so life is good. The fact that you've lost the exception you >> meant to throw is kind of irrelevant at this point. >> >> If you intend to Set properties in an Error object returned by the >> Exception::Error functions and are vulnerable to TerminateExecution then >> you better make sure you got an object back. Any C++ API calls that don't >> use native functions work fine, even after TerminateExecution which is a >> good thing as otherwise it would be a nightmare writing C++ API code where >> TerminateExecution is a possibility. >> >> One thing I'm a little puzzled about is the comment in >> test-thread-termination.cc/ErrorObjectAfterTermination: "// >> TODO(yangguo): crbug/403509. Check for empty handle instead" (any relation >> ;-)). Is there a plan/thought to have Exception::Error return an empty >> handle in the TerminateExecution case (I couldn't locate crbug/403509)? It >> seems that the current behavior of returning Undefined might be better. >> >> Thanks >> >> On Monday, February 15, 2016 at 3:40:25 AM UTC-8, Yang Guo wrote: >>> >>> I guess it would be ok to have it return Local<Object> and crash on >>> anything else inside the API call. It would not break the API since >>> existing users of this should just compile just fine. >>> >>> Accepting patches :) >>> >>> On Thursday, February 11, 2016 at 7:58:06 PM UTC+1, Alex Kodat wrote: >>>> >>>> Would it be a waste of everyone's time if I created an issue and >>>> submitted a change to make Error return Local<Object>? FWIW, my >>>> inclination >>>> would make Factory::NewError return a JSObject and go from there though if >>>> people fee that's a bridge too far, I'd just put a checked cast into >>>> api.cc. It just seems kinda crazy for Factor::NewError to happily return >>>> to >>>> its caller if the situation is so grim it can't return an error object. In >>>> fact, I kinda wonder about it catching an exception thrown by the native >>>> constructor and returning that to the caller. It's really hard to picture >>>> what would be going on in such a scenario and if there's anything sensible >>>> to do beyond falling on one's sword. >>>> >>>> Or maybe this is all too trivial to bother with in which case I >>>> appreciate the discussion, anyway. >>>> >>>> On Thursday, February 11, 2016 at 4:10:39 AM UTC-8, Yang Guo wrote: >>>>> >>>>> The issue is... with other parts of the API, when we return a >>>>> MaybeLocal, it may have thrown an exception, in which process we created >>>>> an >>>>> Error object, and return an empty handle as result. >>>>> >>>>> This case is special. We want to create an Error object. If that >>>>> itself fails, throwing an exception makes no sense, since we cannot >>>>> create >>>>> another Error object. We don't expect this to happen unless bootstrapping >>>>> hasn't finished yet, or we ran out of stack space, or something to that >>>>> effect. >>>>> >>>>> I guess we could turn that into a MaybeLocal<Object>, but I don't >>>>> really think we should break the API for this small detail. That's just >>>>> my >>>>> opinion though. >>>>> >>>>> Yang >>>>> >>>>> On Wednesday, February 10, 2016 at 6:39:44 PM UTC+1, Alex Kodat wrote: >>>>>> >>>>>> First, I'll confess I'm not a huge fan of MaybeLocal but, leaving >>>>>> that aside, even if I accept the utility of MaybeLocal I would expect it >>>>>> to >>>>>> be used for errors for which there's a reasonable hope of recovery and >>>>>> some >>>>>> way of actually testing the recovery code. >>>>>> >>>>>> If Exception::Error returns an empty result, the world (or at least >>>>>> the Isolate) has turned to yogurt and I feel I can't rely on any API >>>>>> calls >>>>>> actually working so it seems like there's not much for me to do other >>>>>> than >>>>>> crashing and I would have preferred the crash closer to the point where >>>>>> the >>>>>> construction of Error actually failed (Factory::NewError). Beyond that, >>>>>> short of an extremely artificial test where I say hack the Error native >>>>>> constructors, I can't see how I could possibly test any error recovery >>>>>> code >>>>>> for an Exception::Error failure. To me this sort of error seems closer >>>>>> to >>>>>> out of storage errors than unusual results errors. >>>>>> >>>>>> All that said, I guess MaybeLocal<Object> Exception::Error is better >>>>>> than Local<Value> or MaybeLocal<Value> as it better documents what an >>>>>> embedder should expect though the former might be accompanied by a >>>>>> comment >>>>>> that the ToLocalChecked on the return value should only ever fail in the >>>>>> event of catastrophic errors so you might not want to expend much >>>>>> bandwidth >>>>>> on worrying about it. >>>>>> >>>>>> On Wednesday, February 10, 2016 at 7:55:01 AM UTC-8, Daniel Vogelheim >>>>>> wrote: >>>>>>> >>>>>>> Generally, the API tries hard to pass errors up. >>>>>>> >>>>>>> I wonder if we should return MaybeLocal<Object>, then. There's been >>>>>>> a huge APi refactoring in the past to deprecate returning empty Locals >>>>>>> (or >>>>>>> Undefined, or so) as error markers, and instead signal all such >>>>>>> failures by >>>>>>> returning an empty MaybeLocal. Not quite what Alex asks for, but IMHO >>>>>>> more >>>>>>> consistent with the remainder of the API. >>>>>>> >>>>>>> >>>>>>> On Wed, Feb 10, 2016 at 4:26 PM, Alex Kodat < >>>>>>> [email protected]> wrote: >>>>>>> >>>>>>>> Thanks for that. I suspected as much. Is v8 really doing embedder's >>>>>>>> a favor by exposing such a catastrophe to them? Presumably, if >>>>>>>> Factory::NewError fails, we're out of storage (which v8 correctly >>>>>>>> doesn't >>>>>>>> do embedders the favor of exposing to them), there's some other >>>>>>>> catastrophic failure (like say the embedder's code has run amok >>>>>>>> clobbering >>>>>>>> the v8 heap), or the native Error constructors have a problem >>>>>>>> (inconceivable except maybe if a developer was fiddling with them). >>>>>>>> >>>>>>>> None of these seem like cases where it's useful to share the pain >>>>>>>> with the embedder code, especially as the situation is presumably that >>>>>>>> the >>>>>>>> embedder code wants to reflect an error to the JS but whoops, all it >>>>>>>> can >>>>>>>> get is an undefined from Exception::Error. Now what? >>>>>>>> >>>>>>>> Could I/should I open an issue for this? Sorry if this is a stupid >>>>>>>> question, I'm still not quite sure when it's appropriate to post to >>>>>>>> the >>>>>>>> list or open an issue. While I know you can code the change about as >>>>>>>> fast >>>>>>>> as I can hit the Post button, I'd be happy to make the change myself. >>>>>>>> FWIW, >>>>>>>> I'd do the type-checking and cast in Factory::NewError and have it >>>>>>>> (them) >>>>>>>> return a JSObject as it seems like it might be useful for other V8 >>>>>>>> code to >>>>>>>> be able to count on NewError giving it an object (like for example, >>>>>>>> the >>>>>>>> JSON parser?). Maybe this is too trivial to waste anyone's bandwidth >>>>>>>> on? >>>>>>>> >>>>>>>> On Wednesday, February 10, 2016 at 5:25:29 AM UTC-8, Yang Guo wrote: >>>>>>>>> >>>>>>>>> This probably never happens, but in case creating the error object >>>>>>>>> fails, undefined is returned. >>>>>>>>> >>>>>>>>> On Monday, February 8, 2016 at 9:03:42 PM UTC+1, Alex Kodat wrote: >>>>>>>>>> >>>>>>>>>> This must have been asked before but can't find an explanation so >>>>>>>>>> ... just curious why Exception::Error et al are declared to have a >>>>>>>>>> Local<Value> result instead of Local<Object>. A not uncommon pattern >>>>>>>>>> is to >>>>>>>>>> create a new Error object and then set some properties on it which >>>>>>>>>> requires >>>>>>>>>> a ->ToObject or Local<Object>::Cast on the Exception::Error result. >>>>>>>>>> Trivial, but it just seems odd that it's necessary. >>>>>>>>>> >>>>>>>>>> Thanks >>>>>>>>>> >>>>>>>>> -- >>>>>>>> -- >>>>>>>> v8-users mailing list >>>>>>>> [email protected] >>>>>>>> http://groups.google.com/group/v8-users >>>>>>>> --- >>>>>>>> You received this message because you are subscribed to the Google >>>>>>>> Groups "v8-users" group. >>>>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>>>> send an email to [email protected]. >>>>>>>> For more options, visit https://groups.google.com/d/optout. >>>>>>>> >>>>>>> >>>>>>> -- >> -- >> v8-users mailing list >> [email protected] <javascript:> >> http://groups.google.com/group/v8-users >> --- >> You received this message because you are subscribed to the Google Groups >> "v8-users" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to [email protected] <javascript:>. >> For more options, visit https://groups.google.com/d/optout. >> > > -- -- v8-users mailing list [email protected] http://groups.google.com/group/v8-users --- You received this message because you are subscribed to the Google Groups "v8-users" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
