TinyIoCContainer Can't replace classes registered as AsPerRequestSingleton() in ReactConfig

[tejacques]

There are really two issues:

1.) The following code in ReactConfig.cs will throw an exception on GetObject():

var container = AssemblyRegistration.Container;
container.Register<IReactEnvironment, ReactEnvironment>()
    .AsPerRequestSingleton();

This can be run only in places where HttpContext.Current is defined, because re-registering a class with AsPerRequestSingleton checks the items in the current HttpContext (I believe to remove it). This could be fixed with an if (null != HttpContext.Current) { /* do things */ }

2.) Even when registering a new ReactEnvironment successfully getting JSX files appears to break. A bare minimum test case I have is this: Follow ReactJS.Net tutorial and have a solution with that set up:

Create a new class:

using React;

namespace ReactDotNetExample
{
    public class CustomReactEnvironment : ReactEnvironment
    {
        public CustomReactEnvironment(
            IJavaScriptEngineFactory engineFactory,
            IReactSiteConfiguration config,
            ICache cache,
            IFileSystem fileSystem,
            IFileCacheHash fileCacheHash
        ) : base(engineFactory, config, cache, fileSystem, fileCacheHash)
        { }
    }
}

Add the following to the HomeController static constructor (HttpContext.Current will be defined here):

var container = AssemblyRegistration.Container;
container.Register<IReactEnvironment, CustomReactEnvironment>()
    .AsPerRequestSingleton();

Grabbing .jsx files will now fail even though CustomReactEnvironment and ReactEnvironment are functionally identical (500 error in browser). Adding the following line:

var container = AssemblyRegistration.Container;
container.Register<IReactEnvironment, CustomReactEnvironment>()
    .AsPerRequestSingleton();
container.Register<IReactEnvironment, ReactEnvironment>()
    .AsPerRequestSingleton();

Works, so the problem is somehow the CustomReactEnvironment class.

For what it's worth, I was trying to see if not disposing/deleting JS Engines per request would speed up server side rendering time, but I couldn't get it to work with a custom environment at all.

[Daniel15]

Hmm, that is really interesting. Could you please post a full stack trace of the 500 error so I can see which class it's failing in? Please use a debug build if possible (clone this repository, run dev-build.bat, and reference the files in bin rather than the NuGet packages)

Perhaps you need to unregister the initial iReactEnvironment before registering the new one? I've never actually tried re-registering a class with TinyIoC.

I was trying to see if not disposing/deleting JS Engines per request would speed up server side rendering time

Unfortunately this can't be easily done with the MSIE engine, as the engine itself is only single-threaded and throws a "catastrophic failure" if you try to touch it from a different thread (even to just dispose it). One solution would be to have a pool of threads, each with one MSIE engine, and delegate the environment to the next available thread in the pool.

Using V8 on Windows may improve performance, I never managed to get it to compile properly though. If you can get V8 version 3.17.16.2 to compile on Windows, you should be able to compile VroomJS and the engine (https://github.com/reactjs/React.NET/tree/master/src/React.JavaScriptEngine.VroomJs) which shares a V8 environment in the whole app (basically there's a single V8 engine for the whole app, and each React environment creates a V8 "context"). Alternatively, VroomJs could be upgraded to support a newer V8 interface, although I don't know enough C++ or enough about V8 to attempt this myself :)

[tejacques]

Here's the stacktrace -- it's not from a linked debug build, but it was still fairly clear, I can grab a better one tonight if necessary:

[NullReferenceException: Object reference not set to an instance of an object.]
   React.Web.TinyIoC.HttpContextLifetimeProvider.GetObject() +45
   React.Web.TinyIoC.HttpContextLifetimeProvider.ReleaseObject() +41
   React.TinyIoC.CustomObjectLifetimeFactory.Dispose() +32
   React.TinyIoC.SafeDictionary`2.set_Item(TKey key, TValue value) +127
   React.TinyIoC.TinyIoCContainer.AddUpdateRegistration(TypeRegistration typeRegistration, ObjectFactoryBase factory) +59
   React.TinyIoC.TinyIoCContainer.RegisterInternal(Type registerType, String name, ObjectFactoryBase factory) +87
   React.TinyIoC.TinyIoCContainer.Register(Type registerType, Type registerImplementation) +99
   React.TinyIoC.TinyIoCContainer.Register() +172
   ReactDotNetExample.ReactConfig.Configure() in d:\Projects\ReactNetExample\ReactDotNetExample\App_Start\ReactConfig.cs:32

The problem appears to be in clearing the old registration -- since it's already a PerRequestSingleton, it tries to clear out the cache from the HttpContext.Current.Items, but HttpContext.Current is null in ReactConfig class since it is initializing and not taken any requests. If I move that code to a controller, it works.

[tejacques]

Hey @Daniel15,

I was finally able to find some time to build the React.NET project, reference it, and hunt these down.

There were two issues preventing me from doing what I'd wanted to do. I think they are both fairly straightforward changes that could be upstreamed, but I'm not sure it really makes sense to upstream the second one. I'm happy to submit a pull request if you'd like, otherwise they are very small changes that I'll include below:

First Change:

In React.Web:
TinyIoCAspNetExtensions.cs

Change GetObject function

from:

public object GetObject()
{
    return HttpContext.Current.Items[_keyName];
}

to:

public object GetObject()
{
    return null != HttpContext.Current
        ? HttpContext.Current.Items[_keyName]
        : null;
}

Change SetObject function

from:

public void SetObject(object value)
{
    HttpContext.Current.Items[_keyName] = value;
}

to:

public void SetObject(object value)
{
    if (null != HttpContext.Current)
    {
        HttpContext.Current.Items[_keyName] = value;
    }
}

Second Change

In React.NET

ReactEnvironment.cs

Change InitialiseEngine function's first line from this:

var thisAssembly = GetType().Assembly;

To this:

var thisAssembly = typeof(ReactEnvironment).Assembly;

I don't know whether you'd want to include this change or not. Really if someone wants their own custom ReactEnvironment, they should copy/paste the base one rather than inheriting it, because none of the functions are virtual anyway. I'm just including it here to document the issue I ran into. What was happening was the Environment was trying to access the React shim js files from the assembly, but GetType().Assembly was returning my project rather than React.NET, and the resource file path was incompatible with that assembly.

For what it's worth, I copied the ReactEnvironment class and simply removed the calls to _engineFactory.DisposeEngineForCurrentThread(), and saw an immense speedup in server-side rendering. Rendering a very simple component server-side (the Tutorial) went from ~200ms to ~11ms

Edit: That said, I also just ran into the issue you were talking about -- I'll try to look into ways to get around it.

[Daniel15]

The change to GetObject is fine, but the change to SetObject is a bit scary in that it essentially completely ignores the call, and the caller doesn't know that (and may expect the object that was set to be correctly disposed at the end of the request). I'll try to see why it's being called in this scenario.

The InitialiseEngine change makes sense and I'm definitely open to accepting that. Additionally, I think it would make sense to make all the methods in ReactEnvironment virtual so they can be overridden.

For what it's worth, I copied the ReactEnvironment class and simply removed the calls to _engineFactory.DisposeEngineForCurrentThread(), and saw an immense speedup in server-side rendering. Rendering a very simple component server-side (the Tutorial) went from ~200ms to ~11ms

The issue here is that it's a memory leak, the JS engine uses unmanaged resources (either the MSIE engine or the V8 engine) and not disposing it at the end of the request means that the unmanaged instance will remain in memory. The best solution to improve server-side rendering performance would be to defer the disposal until after the page has been served to the user (that is, move it to later in the ASP.NET request lifecycle so it occurs after the page has been fully rendered). Not sure if this is possible though.

[tejacques]

Good point about SetObject, I think it was trying to non-lazily initialize the value in the HttpContext's items. The right answer may just be to make it do that lazily and without the null check.

After messing around a lot more with the JS engines, I understand now what you were telling me (and a few others) earlier. I tried changing how the engines are used in a few different ways, but it looks like the problem comes from the engines not being used for a period of time, the threads being collected/removed, and this leaving the engines in a bad state, where if they weren't disposed it causes a memory access exception.

If there's a way to hook into thread collection to dispose the engines that may solve it, but I'm not sure if that's possible. I might try messing around with V8 next -- I'll let you know if I have any luck.

[Daniel15]

Last week I published a React.JavaScriptEngine.ClearScriptV8 package which uses Microsoft's ClearScript library to support V8. Feel free to try it out and let me know how you go. Additionally I just submitted two diffs to change a bunch of methods from private to protected virtual and also implemented the GetType() to typeof(ReactEnvironment) change you mentioned, and these should be in the dev build very soon. I'm going to close this issue but feel free to reopen it or create a new issue if you've got any other questions. Thanks!

[tejacques]

Awesome, thanks @Daniel15!