Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documentation: Use of Modules vs Context #135

Open
jsobell opened this issue May 7, 2018 · 10 comments
Open

Documentation: Use of Modules vs Context #135

jsobell opened this issue May 7, 2018 · 10 comments

Comments

@jsobell
Copy link
Contributor

jsobell commented May 7, 2018

Can you give an overview of the reason for use of Modules vs simply Eval'ing scripts to a Context?
Can we load a set of libraries into Modules?
Can Modules be re-used in different Contexts?
Are there any performance gains or modularity advantages?

@nilproject
Copy link
Owner

nilproject commented May 7, 2018

There is only three (and half) reasons for using Modules:

  1. import and export (i.e. Modules of ECMAScript). It's the main reason.
  2. Preparse code before it will be runned.
  3. Evaluating scripts with embedded time limit mechanism (Run(limit)). But you can implement your own.
  4. Analyze code with embedded static analyzer (yes, and this there is), but it is not so good as js-lint or others and can be used without Module.

Yes, you can load set of libraries into Modules. It's main function of Module. See this example.

No, you cannot reuse modules with different Contexts. Because of some internal optimizations, AST builded with one Context is not recommended to use with another. It is possible with directly using types from namespaces Expressions and Statements, but you should be careful in this case.

No performance penalty or other difference. For the long time since start of development of this library Module.Run was just a wrapper for Context.Eval.

@jsobell
Copy link
Contributor Author

jsobell commented May 7, 2018

OK, that's good, so what is the relationship between Contexts and the Modules? It appears the constructor for Modules only ever refers to the GlobalContext, in which case how can you have separation between instances of the runtime?
For example, we cache Context instances for each unique CustomerProject instance in our code, so we don't allow any interaction between them. To do this we have a custom Context for each. How would Modules be used in this scenario?

@nilproject
Copy link
Owner

In this scenario you need to create GlobalContext for each CustomerProject. Creating regular Context is not enough for isolation. See this example.
A Context of Module is the same as a context created via new Context(), no difference between them. But with Module you cannot create a context that is nested in another regular context (new Context(myContext)), only in GlobalContext via ActivateInCurrentThread().

@jsobell
Copy link
Contributor Author

jsobell commented May 8, 2018

I did see that diagram, but it appears that only one GlobalContext can exist at a time, so the system is not thread-safe. For example:

https://github.com/nilproject/NiL.JS/blob/develop/NiL.JS/Core/Context.cs#L84

Anything referring to Static for anything other than caching purposes is going to cause serious issues if another thread tries to create it's own instance.
For example, if I create a pool of 10 Customer contexts, each sitting on its own thread, handling incoming requests, they must be able to run in isolation. It looks from the code as though there are times when static members are referred to, like this:

return Context.DefaultGlobalContext.GetConstructor(type);

I see the Context class has an Activate method that appears to track the Thread ID, but I'm not sure quite what it's trying to do. Is ActivateInCurrentThread() changing the global context to point to the "current instance", in which case this would cause mayhem if two threads were simultaneously trying to use their own Contexts, or is there another strategy at work here?

@nilproject
Copy link
Owner

This line in JSValue.cs should be fixed. It's a bug.

Because of this lines

[ThreadStatic]
internal static List<Context> currentContextStack;

CurrentContext is thread dependent. See implementation of Activate().
I checked this in multithreaded scenario and already fixed caught bugs.

nilproject added a commit that referenced this issue May 8, 2018
@jsobell
Copy link
Contributor Author

jsobell commented May 8, 2018

Ah, awesome. It was the JSValue reference that was concerning me.
So what is the purpose of ActivateInCurrentThread() ? If the Contexts are thread-safe, why would it ever need to be called?
Also, can you explain in human-talk what that Context thread code is doing? Is it creating a Context per thread?

@nilproject
Copy link
Owner

nilproject commented May 8, 2018

It means that after calling this method each context, created via new Context() (without arguments) in this thread will have as parent GlobalContext that was "activated in current thread". Another thread — another GlobalContext. DefaultGlobalContext is needed for cases, when there is no context activated by the user.

if you do not create a context for each thread, then behavior of contexts will be the same as if there was only one thread. Context do not handle switch between threads.
All code about threads in Context.cs is only for one purpose ­— allow to create different contexts for different threads if user need this.

@jsobell
Copy link
Contributor Author

jsobell commented May 9, 2018

OK, two problems with this approach. Firstly, it appears that the use of the GlobalContext introduces threading issues. If we use a GlobalContext as documented to handle incoming web requests, we get periodic Operation is not valid due to the current state of the object exceptions being raised, probably because the web server creates a new thread for each request.

Secondly, the Context should not be handling any thread functionality itself. Javascript is a single-threaded language, so the only way multiple instances should exist is if a user creates them. If someone creates a WinForms app the UI will be on a different thread to the app, so it's standard practise to have to invoke calls on the other thread. I still don't see the purpose of the Activate and Deactivate methods, as they talk to a global static collection of Context instances, which seems completely wrong.
Likewise with the GlobalContext. Should this actually be a Context, and the existing Context be renamed to ChildContext?
At the moment it seems the idea of the GlobalContext is right, as it contains the base references to types, and every child should reference that, but to do that the child Contexts should be in a collection of that GlobalContext, and never refer to a magic "CurrentGlobalContext" that is shared between every thread.
I can see the issue with things like JSValue. It needs to know the GlobalContext it belongs too, and at the moment it's having to use the magical global variable. This is probably better implemented by using a factory in a Context, so the user says var newval = mycontext.JSValue(99), so the JSValue can retain a reference to the context it belongs to. I know this makes implicit code tricky, but as-is the code cannot be used on a commercial server unless everything everywhere is surrounded in lock statements, and even then we get the error I documented above.

@nilproject
Copy link
Owner

As I understand, you got a InvalidOperationException. Can you give me a stack trace of it?

"Global static collection of Context instances" is a ThreadStatic. Value of this field is different for each thread. This list contains list of all contexts, that was Activated, but not Deactivated yet in current thread only. CurrentGlobalContext as a computed property just take a current executing context and return its GlobalContext.

@jsobell
Copy link
Contributor Author

jsobell commented May 9, 2018

No, the exception is "Operation is not valid due to the current state of the object", and is not thrown in the main library but appears to be thrown by the Kestrel webserver. I'll try and reproduce it tomorrow.
I can't find any decent explanation of what causes it, as everyone refers to things like "too many input forms on your ASP page", which is very unhelpful.

I'll look at the ThreadStatic instance you mention, as I've never used it. For now we're having to create instances of Contexts per project (not thread) and wrap all evaluations in a lock to avoid clashes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants