GNOME Bugzilla – Bug 784713
Switch to native promises
Last modified: 2017-07-21 22:04:01 UTC
Since 1.48 we've included an ES6 promises implementation written externally in JS: the "Lie" library by Calvin Metcalf. Since SpiderMonkey 52, promises are built in to the engine directly. We can remove the external library.
Created attachment 355200 [details] [review] promise: Move to native promises SpiderMonkey 52 includes native promises, so we can remove the embedded JS implementation of promises. Queueing up and resolving of promises is now done inside SpiderMonkey itself, and we must implement some SpiderMonkey extension points in order to get this to integrate with our GLib main loops. The implementation is adapted from what SpiderMonkey does internally in its js::RunJobs() function; see the code in jscntxt.cpp.
Created attachment 355201 [details] [review] promise: Report unhandled rejections A known gotcha with promises is forgetting to attach a .catch() handler, in which case a failed promise is swallowed silently. This will log a warning when that happens.
Review of attachment 355200 [details] [review]: ::: gjs/context.cpp @@ +474,3 @@ + JS::HandleObject job) +{ + return G_SOURCE_REMOVE; I wonder if instead we should explicitly use the source ID and a global variable to determine whether the idle has been already queued or not.
Review of attachment 355201 [details] [review]: Cool!
Created attachment 355222 [details] [review] promise: Report unhandled rejections A known gotcha with promises is forgetting to attach a .catch() handler, in which case a failed promise is swallowed silently. This will log a warning when that happens.
(In reply to Cosimo Cecchi from comment #3) > Review of attachment 355200 [details] [review] [review]: > I wonder if instead we should explicitly use the source ID and a global > variable to determine whether the idle has been already queued or not. I thought about this but was on the fence. I decided not to do it because the state of the job queue being non-empty and the idle being queued should be exactly the same. But maybe it's worth it anyway because of increased clarity, and we could ensure the state is the same with assertions. Updated the second patch to use the new gjs_format_stack_trace() and make better use of unique pointers.
Created attachment 355480 [details] [review] promise: Move to native promises SpiderMonkey 52 includes native promises, so we can remove the embedded JS implementation of promises. Queueing up and resolving of promises is now done inside SpiderMonkey itself, and we must implement some SpiderMonkey extension points in order to get this to integrate with our GLib main loops. The implementation is adapted from what SpiderMonkey does internally in its js::RunJobs() function; see the code in jscntxt.cpp.
Review of attachment 355222 [details] [review]: Looks good
Review of attachment 355480 [details] [review]: ::: gjs/context.cpp @@ +467,3 @@ + /* Uncatchable exceptions are swallowed here - no way to get a handle on + * the main loop to exit it from this idle handler */ + gjs_context->idle_drain_handler = 0; Should you not set this to zero before calling into _gjs_context_run_jobs()? Otherwise that will call g_source_remove() with the ID, but the source callback is already being executed.
(In reply to Cosimo Cecchi from comment #9) > Review of attachment 355480 [details] [review] [review]: > > ::: gjs/context.cpp > @@ +467,3 @@ > + /* Uncatchable exceptions are swallowed here - no way to get a handle on > + * the main loop to exit it from this idle handler */ > + gjs_context->idle_drain_handler = 0; > > Should you not set this to zero before calling into _gjs_context_run_jobs()? > Otherwise that will call g_source_remove() with the ID, but the source > callback is already being executed. I'm not 100% sure, but I think the assertions in enqueue_job will not hold if we do that, since a job might be enqueued while the queue is in the middle of being emptied. This line should actually be unnecessary because run_jobs should always have emptied the queue by the time it finishes; maybe this should be an assert instead?
Created attachment 355904 [details] [review] promise: Report unhandled rejections A known gotcha with promises is forgetting to attach a .catch() handler, in which case a failed promise is swallowed silently. This will log a warning when that happens.
Created attachment 355905 [details] [review] context: Properly create const strings array The std::array object in GjsContext never had its constructor and destructor called. That happens to work, but it depends on the implementation, so do it properly.
The existing patch would crash when compiled with g++-4.9 because I forgot to call the std::unordered_map's constructor properly. This fixes that and makes a similar fix for a std::array in the same struct (which has just happened to work so far.)
Review of attachment 355905 [details] [review]: OK
(In reply to Philip Chimento from comment #10) > This line should actually be unnecessary because run_jobs should always have > emptied the queue by the time it finishes; maybe this should be an assert > instead? My point is not about that line, it's about this flow: - idle source is scheduled with id X - source callback for id X is triggered when reaching mainloop idle point - while the source callback is executing, run_jobs will call g_source_remove(X) (i.e. for the same source X that is executing the callback), since the id X state has not been zeroed yet - the callback returns G_SOURCE_REMOVE for source X again If this is something that GSource gracefully deals with, then no problem, but I wasn't sure whether the source is still considered to be "existing" while its callback is being executed (and it's a programmer error to remove a non-existing source).
Review of attachment 355904 [details] [review]: OK
(In reply to Cosimo Cecchi from comment #15) > (In reply to Philip Chimento from comment #10) > > This line should actually be unnecessary because run_jobs should always have > > emptied the queue by the time it finishes; maybe this should be an assert > > instead? > > My point is not about that line, it's about this flow: > - idle source is scheduled with id X > - source callback for id X is triggered when reaching mainloop idle point > - while the source callback is executing, run_jobs will call > g_source_remove(X) (i.e. for the same source X that is executing the > callback), since the id X state has not been zeroed yet > - the callback returns G_SOURCE_REMOVE for source X again > > If this is something that GSource gracefully deals with, then no problem, > but I wasn't sure whether the source is still considered to be "existing" > while its callback is being executed (and it's a programmer error to remove > a non-existing source). OK, I see. The source is indeed still existing. It's a programmer error to remove a source that's already been removed, but it's not an error to return G_SOURCE_REMOVE from a callback whose source has already been removed. If no objections, I'll push this with the marked line changed to an assert.
Created attachment 355985 [details] [review] Revert "build: Allow compiling without RTTI" This reverts commit eccf3a14b9b0d6aae89baeaa32dd53f07b2b77d7. We don't need to match SpiderMonkey here anymore, since we don't inherit from a SpiderMonkey-defined class.
Attachment 355480 [details] pushed as 4570bb5 - promise: Move to native promises Attachment 355904 [details] pushed as 2c48bee - promise: Report unhandled rejections Attachment 355905 [details] pushed as 810a42a - context: Properly create const strings array
Comment on attachment 355985 [details] [review] Revert "build: Allow compiling without RTTI" I didn't mean to attach this patch, it's part of the ES6 classes refactor, not this bug. This one is now fixed and should be closed.