Promise lets go again

[twop]

This is a draft PR.

Upd: not draft anymore

reapplied initial changes from #643. But I only kept scheduling logic in Engine and test s, thus this is a complete rewrite internally.

Done:

• Updated 262 tests from the spec main branch
• Ability to run tests on Mac (there were file path issues and time zones handling platform differences)
• All sync Promise 262 tests are passing
• Old tests are reworked according to spec and fixed.

Left:

• Interop/public API for creating Promises. Right now you can't create a Promise instance in C# as a library user.
• Potentially "opt-in" into Promise support. There is a lock right now in Execute method wether you use Promises or not. I think that this should be optional and opt-in (you don't pay the price by default).

All feedback is welcomed.

Note: I believe that Promise should not interop with Task smoothly, at least not initially. I think a better version will be something along these lines

var  (resolve, reject) = PromiseInstance.CreateManual(engine);

// then resolve it later like so

resolve(new JsString("resolved!")); // note that type of resolve is Action<JsValue>

Upd: all what is left is done now and PR is "marked ready for review"

[lahma]

Initial thought is that maybe it would make sense to bring the updated test suite as a separate PR so this one would be easier to concentrate on, now it's hiding the actual thing be having so many files.

[twop]

Initial thought is that maybe it would make sense to bring the updated test suite as a separate PR so this one would be easier to concentrate on, now it's hiding the actual thing be having so many files.

There is a way to filter by file extensions, I had the same concern but just reviewed myself by using the .js filter. Was manageable. Please let me know if it works for you.

I couldn't keep the old tests and enable them at the same time because some of them were not working according to the spec

[lahma]

There is a way to filter by file extensions, I had the same concern but just reviewed myself by using the .js filter. Was manageable. Please let me know if it works for you.

I couldn't keep the old tests and enable them at the same time because some of them were not working according to the spec

Ah, TIL, thanks for the tip. Let's try out this route.

[lahma]

Overall looking great, pedantic follow of the spec and good comments. Things noted are mostly about hiding as much as possible (internal) as it gives us more wiggle room. I think a thing to figure out is the started task, how do we make sure it's not forgotten and as it's per engine there can be a lot of them created.

[lahma]

internal probably

[twop]

I wanted to use it in public API (which I haven't flushed out yet)

[twop]

@lahma do you understand what is not working now in CI?

[sebastienros]

All tests pas but exit code is 1, weird. Will check tomorrow. Could be an issue in the runner that doesn't accept skipped tests, or an issue that we don't see in the logs, or a breaking change in the sdk.

[lahma]

At least for me locally Jint.Tests hangs, might be that xUnit killed the run.

[twop]

@sebastienros @lahma I added draft version of public API for creating Promises. There is a test that demonstrates it: RegisterPromise_CalledWithinExecuteWithEventLoop_ResolvesCorrectly

[twop]

thoughts about public API are more than welcome :)

[lahma]

I might be way off base here, but some thoughts, feel free to shoot them down (I'm just writing from end-user perspective). EventLoop is implementation details so I don't think it's good thing to expose in API. This is partly pseudo-code, using things that might not be there (await is shorter example).

Engine engine = new Engine(options => options.EnablePromiseEvaluation()); // if really needed
JsValue result1 = engine.Evaluate("1 + 1");

// translates to event loop if enabled
JsValue result2 = engine.Evaluate("await 1;"); , 

// above translates to something like, ideally enables event loop when first seen that required
-> engine.InternalEventLoop(() => manualResetEvent.Set(true)); manualResetEvent.Wait();

// then there's possibility maybe later to configure per call, not part of this PR
engine.Evaluate("await 1", options => options.EnablePromiseEvaluation = true);

So trying to hide as much about the internals for now, I'm leaving postfix etc out of scope now. The evaluate allow configuring the execution context parameter we've talked before, which would flow through all calls etc, call level configuration.

Having to manually state EnablePromiseEvaluation (enable event loop) seems odd to me, we could just automatically enable when first seeing need to do so and then if started, always wait to end? Probably with timeout.

[twop]

@lahma I think usage of EventLoop is fundamentally different than just Execute, specifically it assumes long running tasks. I think because it is fundamentally different from synchronous "input -> output" of normal scripting that it justifies explicit API for it.

I'm open to any suggestions and ideas but I think I would love to land the current public API with the caveat that it is experimental and subject to change

[ChrML]

I don't think opt-in Promise support is necessary. It should be enabled by default.

A lock in a single-threaded environment is very cheap as they will complete immediately with spinlocks, without involving the OS. Locks only have significant negative impact in multithreaded environments in areas with concurrency.