Use of [ThreadStatic] in ReactComponent

[xecrets]

I have been working with solving problems with high memory pressure, garbage collection and large object heap fragmentation in a high traffic production site, and among other things I've noticed some behaviors with React.NET that I'd like to discuss.

This following is not a bug in the sense something is provenly broken, but there is potential to break and also there is apparent potential for the intent to fail, and cause inefficiency rather than the intended optimization.

In the latest code - and since a long time, ReactComponent uses [ThreadStatic] like this:

[ThreadStatic]
private static StringWriter _sharedStringWriter;

The intent seems quite clear - to re-use the underlying StringBuilder used by the StringWriter, and thus reducing memory pressure, garbage collection and possibly large object heap fragmentation. That's great!

However, as far as I understand it, in ASP.NET request threads are taken from the thread pool and once a request is finished the request is returned to the thread pool - but this will not affect TLS, Thread Local Storage which is the underlying mechanism used for ThreadStaticAttribute-marked fields. The field will remain in play for the lifetime of the thread, which likely is the same as the lifetime of the worker process.

Under traditional circumstances reasonable assumptions are:

1. A request is served by a single thread
2. There are are a fairly limited number of threads in the thread pool

However, neither of these assumptions necessarily hold any longer.

Although it's unlikely that code is written causing a request to be served by different threads causing problems with assumption 1, it's possible to write such code and the likelyhood increases with increased pervasiveness of async programming.

The second assumption is no longer quite true, ASP.NET has over time relaxed it's reluctance to have many threads in the thread pool, and I've personally seen thousands of threads in the thread pool in a default-configured production machine under high load.

The net effect is thus that there is potential for scary bugs, and also a potential that with a large number of threads in the thread pool, a lot of memory will be locked up.

In general my feeling is that [TreadStatic] and ASP.NET and thread pools just feels like a dubious practice.

Since there is a limit on the number of engines spun up, there can never really be a need for more than that number of '_sharedStringWriters' I think. It would thus perhaps make sense to let the engine own it, and expose it via the IReactEnvironment implementation. The environment is kept track of on a per-request basis, using HttpContext.Items as the underlying mechanism, and that will work regardless. If engine pooling is enabled, then the '_sharedStringWriters' will also be pooled to the same extent.

I may have missed something, or misunderstood something, but this is intended as a starting point for a discussion. If the discussion leads to something, I'll be happy to try and implement it and make a pull request.

I have some other thoughts about related issues that I'll bring up separately.

[Daniel15]

This change was originally made in #532 and #522. I'm not very familiar with thread pool behaviour but maybe the author of the PR (@DaniilSokolyuk) has some insights into this.

[DaniilSokolyuk]

@xecrets
Hi! Yes, the number of threads can really be large (threadpool starvation) this most often happens due to synchronous code. The idea of having one StringWriter for one engine sounds good