microsoft / ClearScript

A library for adding scripting to .NET applications. Supports V8 (Windows, Linux, macOS) and JScript/VBScript (Windows).
https://microsoft.github.io/ClearScript/
MIT License
1.74k stars 148 forks source link

ScriptEngine.Current defined as AsyncLocal instead of ThreadStatic #449

Closed dalibor-florian closed 1 year ago

dalibor-florian commented 1 year ago

Hi ClearScriptLib. Would it be possible to redefined ScriptEngine.Current property redefined as AsyncLocal instead of ThreadStatic?

I am exposing method for listing items in storage to script. Return type is defined as AsyncEnumerable. ILocation is supertype for IFile and ILocation. On items from enumerable i need to call ToRestrictedHostObject to be able use specific methods on IFile and IDirectory.

So i have this mapping extension which does that:

 IAsyncEnumerable<ILocation> list = someSource();
 await foreach (var item in list)
  {
      yield return item switch
      {
          IFile file => file.ToRestrictedHostObject<IFile>(),
          IDirectory directory => directory.ToRestrictedHostObject<IDirectory>(),
          _ => throw new NotSupportedException($"Unsupported inheritor of ILocation. Type: {item.GetType().FullName}")
      };
  }

Problem is that after async call i am returned to different thread. I am running on asp.net core 6.0, so there is no synchornization context. I have workaround it by keeping original value of ScriptEngine.Current in variable before async call, but i think that it would be better if the Current property was kept across async flow using AsyncLocal.

ClearScriptLib commented 1 year ago

Hi @dalibor-florian,

That's an interesting idea, but we don't believe that reimplementing ScriptEngine.Current on top of async-local storage would be a good way to facilitate scenarios such as yours.

The problem has less to do with behavior than implication. Today, if Current returns a valid engine, it's always safe to re-enter that engine on the current thread. The change you're proposing would invalidate that assumption.

Also, retrieving the engine from a closure – as you're doing with your workaround, presumably – is likely to be more efficient than accessing async-local storage.

A different property – perhaps AsyncLocalCurrent – might make sense, but then ToRestrictedHostObject() would have no basis for selecting that property over Current. We could provide additional, parallel methods, but the increase in ClearScript's API surface would probably not be worth it in this case.

Please send any additional thoughts or suggestions. Thanks!

ClearScriptLib commented 1 year ago

Please reopen this issue if you have additional thoughts or questions about this topic. Thank you!