ClearScript 7.0 RC3: Fixed more task-promise conversion issues (GitHu…

…b Issue #198).
microsoft · Oct 23, 2020 · b126818 · b126818
1 parent 3fbb65b
commit b126818
Show file tree

Hide file tree

Showing 13 changed files with 140 additions and 76 deletions.
diff --git a/ClearScript/Exports/VersionSymbols.h b/ClearScript/Exports/VersionSymbols.h
@@ -7,5 +7,5 @@
 
 #define CLEARSCRIPT_VERSION_STRING "7.0.0"
 #define CLEARSCRIPT_VERSION_COMMA_SEPARATED 7,0,0
-#define CLEARSCRIPT_VERSION_STRING_INFORMATIONAL "7.0.0-rc2"
+#define CLEARSCRIPT_VERSION_STRING_INFORMATIONAL "7.0.0-rc3"
 #define CLEARSCRIPT_FILE_FLAGS VS_FF_PRERELEASE
diff --git a/ClearScript/JavaScript/IJavaScriptEngine.cs b/ClearScript/JavaScript/IJavaScriptEngine.cs
@@ -1,12 +1,17 @@
-using System.Threading.Tasks;
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license.
+
+using System.Threading.Tasks;
 
 namespace Microsoft.ClearScript.JavaScript
 {
     internal interface IJavaScriptEngine
     {
         uint BaseLanguageVersion { get; }
 
-        void CompletePromiseWithResult<T>(Task<T> task, object resolve, object reject);
-        void CompletePromise(Task task, object resolve, object reject);
+        object CreatePromiseForTask<T>(Task<T> task);
+        object CreatePromiseForTask(Task task);
+
+        Task<object> CreateTaskForPromise(ScriptObject promise);
     }
 }
diff --git a/ClearScript/JavaScript/JavaScriptExtensions.cs b/ClearScript/JavaScript/JavaScriptExtensions.cs
@@ -12,8 +12,6 @@ namespace Microsoft.ClearScript.JavaScript
     /// </summary>
     public static class JavaScriptExtensions
     {
-        private delegate void Executor(object resolve, object reject);
-
         /// <summary>
         /// Converts a <see cref="Task{TResult}"/> instance to a
         /// <see href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise">promise</see>
@@ -47,11 +45,7 @@ public static object ToPromise<TResult>(this Task<TResult> task, ScriptEngine en
                 throw new NotSupportedException("The script engine does not support promises");
             }
 
-            return engine.Script.EngineInternal.createPromise(new Executor((resolve, reject) =>
-            {
-                Action<Task<TResult>> continuation = thisTask => javaScriptEngine.CompletePromiseWithResult(thisTask, resolve, reject);
-                task.ContinueWith(continuation, TaskContinuationOptions.ExecuteSynchronously);
-            }));
+            return javaScriptEngine.CreatePromiseForTask(task);
         }
 
         /// <summary>
@@ -85,11 +79,7 @@ public static object ToPromise(this Task task, ScriptEngine engine)
                 throw new NotSupportedException("The script engine does not support promises");
             }
 
-            return engine.Script.EngineInternal.createPromise(new Executor((resolve, reject) =>
-            {
-                Action<Task> continuation = thisTask => javaScriptEngine.CompletePromise(thisTask, resolve, reject);
-                task.ContinueWith(continuation, TaskContinuationOptions.ExecuteSynchronously);
-            }));
+            return javaScriptEngine.CreatePromiseForTask(task);
         }
 
         /// <summary>
@@ -104,33 +94,18 @@ public static Task<object> ToTask(this object promise)
             MiscHelpers.VerifyNonNullArgument(promise, "promise");
 
             var scriptObject = promise as ScriptObject;
-            if ((scriptObject == null) || !scriptObject.Engine.Script.EngineInternal.isPromise(promise))
+            if (scriptObject == null)
             {
                 throw new ArgumentException("The object is not a promise", nameof(promise));
             }
 
-            var source = new TaskCompletionSource<object>();
-
-            Action<object> onResolved = result =>
-            {
-                source.SetResult(result);
-            };
-
-            Action<object> onRejected = error =>
+            var javaScriptEngine = scriptObject.Engine as IJavaScriptEngine;
+            if ((javaScriptEngine == null) || (javaScriptEngine.BaseLanguageVersion < 6))
             {
-                try
-                {
-                    scriptObject.Engine.Script.EngineInternal.throwValue(error);
-                }
-                catch (Exception exception)
-                {
-                    source.SetException(exception);
-                }
-            };
-
-            scriptObject.InvokeMethod("then", onResolved, onRejected);
+                throw new NotSupportedException("The script engine does not support promises");
+            }
 
-            return source.Task;
+            return javaScriptEngine.CreateTaskForPromise(scriptObject);
         }
     }
 }
diff --git a/ClearScript/Properties/AssemblyInfo.cs b/ClearScript/Properties/AssemblyInfo.cs
@@ -17,13 +17,13 @@
 [assembly: ComVisible(false)]
 [assembly: AssemblyVersion("7.0.0")]
 [assembly: AssemblyFileVersion("7.0.0")]
-[assembly: AssemblyInformationalVersion("7.0.0-rc2")]
+[assembly: AssemblyInformationalVersion("7.0.0-rc3")]
 
 namespace Microsoft.ClearScript.Properties
 {
     internal static class ClearScriptVersion
     {
         public const string Triad = "7.0.0";
-        public const string Informational = "7.0.0-rc2";
+        public const string Informational = "7.0.0-rc3";
     }
 }
diff --git a/ClearScript/V8/V8ScriptEngine.cs b/ClearScript/V8/V8ScriptEngine.cs
@@ -30,7 +30,6 @@ public sealed class V8ScriptEngine : ScriptEngine, IJavaScriptEngine
         #region data
 
         private static readonly DocumentInfo initScriptInfo = new DocumentInfo(MiscHelpers.FormatInvariant("{0} [internal]", nameof(V8ScriptEngine)));
-        [ThreadStatic] private static bool bypassTaskPromiseConversion;
 
         private readonly V8ScriptEngineFlags engineFlags;
         private readonly V8ContextProxy proxy;
@@ -288,23 +287,25 @@ function construct() {
                                     return value instanceof savedPromise;
                                 },
 
-                                completePromiseWithResult: function (task, resolve, reject) {
+                                completePromiseWithResult: function (getResult, resolve, reject) {
                                     try {
-                                        resolve(task.Result);
+                                        resolve(getResult());
                                     }
                                     catch (exception) {
                                         reject(exception);
                                     }
+                                    return undefined;
                                 },
 
-                                completePromise: function (task, resolve, reject) {
+                                completePromise: function (wait, resolve, reject) {
                                     try {
-                                        task.Wait();
+                                        wait();
                                         resolve();
                                     }
                                     catch (exception) {
                                         reject(exception);
                                     }
+                                    return undefined;
                                 },
 
                                 throwValue: function (value) {
@@ -1101,6 +1102,26 @@ private void OnContinuationTimer(Timer timer)
             }
         }
 
+        private object CreatePromise(Action<object, object> executor)
+        {
+            VerifyNotDisposed();
+            var v8Script = (V8ScriptItem)script;
+            var v8Internal = (V8ScriptItem)v8Script.GetProperty("EngineInternal");
+            return V8ScriptItem.Wrap(this, v8Internal.InvokeMethod(false, "createPromise", executor));
+        }
+
+        private void CompletePromise<T>(Task<T> task, object resolve, object reject)
+        {
+            Func<T> getResult = () => task.Result;
+            Script.EngineInternal.completePromiseWithResult(getResult, resolve, reject);
+        }
+
+        private void CompletePromise(Task task, object resolve, object reject)
+        {
+            Action wait = task.Wait;
+            Script.EngineInternal.completePromise(wait, resolve, reject);
+        }
+
         #endregion
 
         #region ScriptEngine overrides (public members)
@@ -1289,7 +1310,7 @@ internal override object MarshalToScript(object obj, HostItemFlags flags)
                 return obj;
             }
 
-            if (engineFlags.HasFlag(V8ScriptEngineFlags.EnableTaskPromiseConversion) && !bypassTaskPromiseConversion)
+            if (engineFlags.HasFlag(V8ScriptEngineFlags.EnableTaskPromiseConversion))
             {
                 // .NET Core async functions return Task subclass instances that trigger result wrapping
 
@@ -1303,17 +1324,11 @@ internal override object MarshalToScript(object obj, HostItemFlags flags)
                 {
                     if (testObject.GetType().IsAssignableToGenericType(typeof(Task<>), out var typeArgs))
                     {
-                        using (Scope.Create(() => MiscHelpers.Exchange(ref bypassTaskPromiseConversion, true), oldValue => bypassTaskPromiseConversion = oldValue))
-                        {
-                            obj = typeof(TaskConverter<>).MakeSpecificType(typeArgs).InvokeMember("ToPromise", BindingFlags.InvokeMethod | BindingFlags.Public | BindingFlags.Static, null, null, new[] {testObject, this});
-                        }
+                        obj = typeof(TaskConverter<>).MakeSpecificType(typeArgs).InvokeMember("ToPromise", BindingFlags.InvokeMethod | BindingFlags.Public | BindingFlags.Static, null, null, new[] {testObject, this});
                     }
                     else if (testObject is Task task)
                     {
-                        using (Scope.Create(() => MiscHelpers.Exchange(ref bypassTaskPromiseConversion, true), oldValue => bypassTaskPromiseConversion = oldValue))
-                        {
-                            obj = task.ToPromise(this);
-                        }
+                        obj = task.ToPromise(this);
                     }
                 }
             }
@@ -1378,12 +1393,9 @@ internal override object MarshalToHost(object obj, bool preserveHostTarget)
             }
 
             var scriptItem = V8ScriptItem.Wrap(this, obj);
-            if (engineFlags.HasFlag(V8ScriptEngineFlags.EnableTaskPromiseConversion) && !bypassTaskPromiseConversion && (obj is IV8Object v8Object) && v8Object.IsPromise())
+            if (engineFlags.HasFlag(V8ScriptEngineFlags.EnableTaskPromiseConversion) && (obj is IV8Object v8Object) && v8Object.IsPromise())
             {
-                using (Scope.Create(() => MiscHelpers.Exchange(ref bypassTaskPromiseConversion, true), oldValue => bypassTaskPromiseConversion = oldValue))
-                {
-                    return scriptItem.ToTask();
-                }
+                return scriptItem.ToTask();
             }
 
             return scriptItem;
@@ -1505,20 +1517,51 @@ protected override void Dispose(bool disposing)
 
         uint IJavaScriptEngine.BaseLanguageVersion => 8;
 
-        void IJavaScriptEngine.CompletePromiseWithResult<T>(Task<T> task, object resolve, object reject)
+        object IJavaScriptEngine.CreatePromiseForTask<T>(Task<T> task)
         {
-            using (Scope.Create(() => MiscHelpers.Exchange(ref bypassTaskPromiseConversion, true), oldValue => bypassTaskPromiseConversion = oldValue))
+            return CreatePromise((resolve, reject) =>
             {
-                Script.EngineInternal.completePromiseWithResult(task, resolve, reject);
-            }
+                task.ContinueWith(_ => CompletePromise(task, resolve, reject), TaskContinuationOptions.ExecuteSynchronously);
+            });
         }
 
-        void IJavaScriptEngine.CompletePromise(Task task, object resolve, object reject)
+        object IJavaScriptEngine.CreatePromiseForTask(Task task)
         {
-            using (Scope.Create(() => MiscHelpers.Exchange(ref bypassTaskPromiseConversion, true), oldValue => bypassTaskPromiseConversion = oldValue))
+            return CreatePromise((resolve, reject) =>
             {
-                Script.EngineInternal.completePromise(task, resolve, reject);
+                task.ContinueWith(_ => CompletePromise(task, resolve, reject), TaskContinuationOptions.ExecuteSynchronously);
+            });
+        }
+
+        Task<object> IJavaScriptEngine.CreateTaskForPromise(ScriptObject promise)
+        {
+            if (!(promise is V8ScriptItem v8Promise) || !v8Promise.IsPromise())
+            {
+                throw new ArgumentException("The object is not a V8 promise", nameof(promise));
             }
+
+            var source = new TaskCompletionSource<object>();
+
+            Action<object> onResolved = result =>
+            {
+                source.SetResult(result);
+            };
+
+            Action<object> onRejected = error =>
+            {
+                try
+                {
+                    Script.EngineInternal.throwValue(error);
+                }
+                catch (Exception exception)
+                {
+                    source.SetException(exception);
+                }
+            };
+
+            v8Promise.InvokeMethod(false, "then", onResolved, onRejected);
+
+            return source.Task;
         }
 
         #endregion

diff --git a/ClearScript/V8/V8ScriptItem.cs b/ClearScript/V8/V8ScriptItem.cs
@@ -76,6 +76,24 @@ public static object Wrap(V8ScriptEngine engine, object obj)
             return obj;
         }
 
+        public bool IsPromise()
+        {
+            return target.IsPromise();
+        }
+
+        public object InvokeMethod(bool marshalResult, string name, params object[] args)
+        {
+            VerifyNotDisposed();
+
+            var result = engine.ScriptInvoke(() => target.InvokeMethod(name, engine.MarshalToScript(args)));
+            if (marshalResult)
+            {
+                return engine.MarshalToHost(result, false);
+            }
+
+            return result;
+        }
+
         private void VerifyNotDisposed()
         {
             if (disposedFlag.IsSet)
@@ -255,8 +273,7 @@ public override object Invoke(bool asConstructor, params object[] args)
 
         public override object InvokeMethod(string name, params object[] args)
         {
-            VerifyNotDisposed();
-            return engine.MarshalToHost(engine.ScriptInvoke(() => target.InvokeMethod(name, engine.MarshalToScript(args))), false);
+            return InvokeMethod(true, name, args);
         }
 
         #endregion

diff --git a/ClearScript/Windows/JScriptEngine.Unix.cs b/ClearScript/Windows/JScriptEngine.Unix.cs
@@ -90,12 +90,17 @@ protected JScriptEngine(string progID, string name, string fileNameExtensions, W
 
         uint IJavaScriptEngine.BaseLanguageVersion => 3;
 
-        void IJavaScriptEngine.CompletePromiseWithResult<T>(Task<T> task, object resolve, object reject)
+        object IJavaScriptEngine.CreatePromiseForTask<T>(Task<T> task)
         {
             throw new NotImplementedException();
         }
 
-        void IJavaScriptEngine.CompletePromise(Task task, object resolve, object reject)
+        object IJavaScriptEngine.CreatePromiseForTask(Task task)
+        {
+            throw new NotImplementedException();
+        }
+
+        Task<object> IJavaScriptEngine.CreateTaskForPromise(ScriptObject promise)
         {
             throw new NotImplementedException();
         }

diff --git a/ClearScript/Windows/JScriptEngine.cs b/ClearScript/Windows/JScriptEngine.cs
@@ -299,12 +299,17 @@ internal override object Execute(UniqueDocumentInfo documentInfo, string code, b
 
         uint IJavaScriptEngine.BaseLanguageVersion => 3;
 
-        void IJavaScriptEngine.CompletePromiseWithResult<T>(Task<T> task, object resolve, object reject)
+        object IJavaScriptEngine.CreatePromiseForTask<T>(Task<T> task)
         {
             throw new NotImplementedException();
         }
 
-        void IJavaScriptEngine.CompletePromise(Task task, object resolve, object reject)
+        object IJavaScriptEngine.CreatePromiseForTask(Task task)
+        {
+            throw new NotImplementedException();
+        }
+
+        Task<object> IJavaScriptEngine.CreateTaskForPromise(ScriptObject promise)
         {
             throw new NotImplementedException();
         }

diff --git a/ClearScriptBenchmarks/Properties/AssemblyInfo.cs b/ClearScriptBenchmarks/Properties/AssemblyInfo.cs
@@ -13,4 +13,4 @@
 [assembly: ComVisible(false)]
 [assembly: AssemblyVersion("7.0.0")]
 [assembly: AssemblyFileVersion("7.0.0")]
-[assembly: AssemblyInformationalVersion("7.0.0-rc2")]
+[assembly: AssemblyInformationalVersion("7.0.0-rc3")]
diff --git a/ClearScriptConsole/Properties/AssemblyInfo.cs b/ClearScriptConsole/Properties/AssemblyInfo.cs
@@ -13,4 +13,4 @@
 [assembly: ComVisible(false)]
 [assembly: AssemblyVersion("7.0.0")]
 [assembly: AssemblyFileVersion("7.0.0")]
-[assembly: AssemblyInformationalVersion("7.0.0-rc2")]
+[assembly: AssemblyInformationalVersion("7.0.0-rc3")]