Any SpiderMonkey embedding that uses JS_THREADSAFE could implement an iterator with native code and, with the right mutual exclusion, make it both thread-safe and multi-threaded -- the latter meaning usable from several threads racing to call .next() at once. But by default, the native iterator and generator types should not take on the cost imposed by MT accesses. They need at most to be thread-safe (as in, you can't crash or subvert memory safety or other safety properties by abusing them from two threads running concurrently). More on how to do this. Not necessarily a 1.8.1 blocker but targeting at 1.8.1, to make sure there aren't problems we need to fix even in Firefox 2. /be

A trivial way to fix this for now would be to say that calling next, send, throw and close is allowed only from the same thread that created the generator() instance and calling them from any other thread explicitly triggers a runtime exception. And E4X cursors can be fixed similarly. To implement this one would need to store the original thread in JSGenerator.

Here is a trivial implementation of creator-thread-only idea build on top of the patch from bug 349320.

Comment on attachment 234589 [details] [diff] [review] Implementation Neat, simple too. Does the patch need updating to track the "Generator cleanup" bug's final patch? >--- .pc/no_multithreading_generators.patch/js/src/js.msg 2006-08-19 03:02:33.000000000 +0200 >+++ js/src/js.msg 2006-08-19 17:00:19.000000000 +0200 >@@ -290,8 +290,9 @@ MSG_DEF(JSMSG_WRONG_CONSTRUCTOR, 20 > MSG_DEF(JSMSG_BAD_GENERATOR_RETURN, 208, 1, JSEXN_TYPEERR, "generator function {0} returns a value") > MSG_DEF(JSMSG_BAD_ANON_GENERATOR_RETURN, 209, 0, JSEXN_TYPEERR, "anonymous generator function returns a value") > MSG_DEF(JSMSG_NAME_AFTER_FOR_PAREN, 210, 0, JSEXN_SYNTAXERR, "missing name after for (") > MSG_DEF(JSMSG_IN_AFTER_FOR_NAME, 211, 0, JSEXN_SYNTAXERR, "missing in after for") > MSG_DEF(JSMSG_BAD_ITERATOR_RETURN, 212, 2, JSEXN_TYPEERR, "{0}.{1} returned a primitive value") > MSG_DEF(JSMSG_KEYWORD_NOT_NS, 213, 0, JSEXN_SYNTAXERR, "keyword is used as namespace") > MSG_DEF(JSMSG_BAD_GENERATOR_YIELD, 214, 1, JSEXN_TYPEERR, "yield from closing generator {0}") > MSG_DEF(JSMSG_NESTING_GENERATOR, 215, 1, JSEXN_TYPEERR, "already executing generator {0}") >+MSG_DEF(JSMSG_BAD_GENERATOR_THREAD, 216, 1, JSEXN_TYPEERR, "calling generator operation from a thread that did not create the instance of {0}") How about shorter message that names the operator? "calling {0} from a thread that did not create the generator {1}" /be

(In reply to comment #3) > (From update of attachment 234589 [details] [diff] [review] [edit]) > Neat, simple too. Does the patch need updating to track the "Generator > cleanup" bug's final patch? Yes. > How about shorter message that names the operator? > > "calling {0} from a thread that did not create the generator {1}" For future compatibility I think it is better to put in the updated patch just "calling {0} from a thread that do not own {1}". I would like to extend the same thread-protection to cover xml cursors (straightforward) and iterators (messy since the iterator itself does not have space to store the thread), and reusing the message again would be handy.

(In reply to comment #4) > (In reply to comment #3) > > How about shorter message that names the operator? > > > > "calling {0} from a thread that did not create the generator {1}" > > For future compatibility I think it is better to put in the updated patch just > "calling {0} from a thread that do not own {1}". Ok -- I guess this message would named be JSMSG_BAD_THREAD_ACCESS or some such instead. That suggests further generalization, below. > I would like to extend the same thread-protection to cover xml cursors > (straightforward) and iterators (messy since the iterator itself does not have > space to store the thread), and reusing the message again would be handy. When compiling JS_THREADSAFE, all native objects have OBJ_SCOPE(obj)->ownercx, which if null means the object has been accessed by (contexts used by) two or more threads in overlapping requests. Perhaps we could use OBJ_SCOPE(obj)->ownercx and a SCOPE_SINGLE_THREAD flag to enforce ownercx invariance, throwing the JSMSG_BAD_THREAD_ACCESS on any attempt to call any object operation from a non-owning thread? /be

We would want to avoid conflacting context (ownercx) and thread (ownercx->thread) so as to allow accesses from the same thread using two different contexts. /be

The patch protects both iterator and generators against multi-threading access via checking for OBJ_SCOPE(obj)->ownercx. AFAICS the check can not be made generic with a special flag on 1.8.1 branch as there is no space left for yet another function flag.

Comment on attachment 236058 [details] [diff] [review] Implementation v2 >Index: js.msg >=================================================================== >RCS file: /cvsroot/mozilla/js/src/js.msg,v >retrieving revision 3.67 >diff -U7 -p -r3.67 js.msg >--- js.msg 26 Aug 2006 20:24:45 -0000 3.67 >+++ js.msg 30 Aug 2006 09:28:02 -0000 >@@ -290,7 +290,8 @@ MSG_DEF(JSMSG_WRONG_CONSTRUCTOR, 20 > MSG_DEF(JSMSG_BAD_GENERATOR_RETURN, 208, 1, JSEXN_TYPEERR, "generator function {0} returns a value") > MSG_DEF(JSMSG_BAD_ANON_GENERATOR_RETURN, 209, 0, JSEXN_TYPEERR, "anonymous generator function returns a value") > MSG_DEF(JSMSG_NAME_AFTER_FOR_PAREN, 210, 0, JSEXN_SYNTAXERR, "missing name after for (") > MSG_DEF(JSMSG_IN_AFTER_FOR_NAME, 211, 0, JSEXN_SYNTAXERR, "missing in after for") > MSG_DEF(JSMSG_BAD_ITERATOR_RETURN, 212, 2, JSEXN_TYPEERR, "{0}.{1} returned a primitive value") > MSG_DEF(JSMSG_KEYWORD_NOT_NS, 213, 0, JSEXN_SYNTAXERR, "keyword is used as namespace") > MSG_DEF(JSMSG_BAD_GENERATOR_YIELD, 214, 1, JSEXN_TYPEERR, "yield from closing generator {0}") >+MSG_DEF(JSMSG_ONLY_ONE_THREAD, 215, 2, JSEXN_TYPEERR, "calling {0} on object {1} that is accessed from multiple threads.") Suggest "method {0} called on {1} from concurrently executing threads". >Index: jsiter.c >=================================================================== >RCS file: /cvsroot/mozilla/js/src/jsiter.c,v >retrieving revision 3.37 >diff -U7 -p -r3.37 jsiter.c >--- jsiter.c 29 Aug 2006 20:37:49 -0000 3.37 >+++ jsiter.c 30 Aug 2006 09:28:02 -0000 >@@ -119,14 +119,52 @@ JSClass js_IteratorClass = { > JS_PropertyStub, JS_PropertyStub, JS_PropertyStub, JS_PropertyStub, > JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, JS_FinalizeStub, > JSCLASS_NO_OPTIONAL_MEMBERS > }; > > #if JS_HAS_GENERATORS > >+#ifdef JS_THREADSAFE >+# define CHECK_SINGLE_THREADED(cx, obj, argv, whenMultiThreded) \ s/Threded/Threaded/ To make this generic, why not use a SCOPE_SINGLE_THREADED flag instead of a function flag? The locus of single-threaded-ness is the object, not one or some of its methods. /be

(In reply to comment #8) > To make this generic, why not use a SCOPE_SINGLE_THREADED flag instead of a > function flag? The locus of single-threaded-ness is the object, not one or > some of its methods. But what about someOtherGeneratorInstance.next.call(openedGeneratorOnAnotherThread) ? If the flag belongs to the class, it means that for every function call it is necessary to access the class of this. Do we want such check?

(In reply to comment #9) > (In reply to comment #8) > > To make this generic, why not use a SCOPE_SINGLE_THREADED flag instead of a > > function flag? The locus of single-threaded-ness is the object, not one or > > some of its methods. > > But what about > someOtherGeneratorInstance.next.call(openedGeneratorOnAnotherThread) ? The object referenced by openedGeneratorOnAnotherThread will be no longer in use by a request active on that other thread, or else will be shared among threads with a thin lock instead of the ownercx flyweight lock. In the first case, the access will be allowed on the current thread, and I think it should be. In the second, per your patch's null test of ownercx_, access will be denied. IOW, when requests take turns accessing an iterator object that somehow becamse accessible among threads, access is allowed. But if a request should try to lock the object while it's in flyweight ownership use by another thread running in a request of its own, the object will be switched to thin lock ownership, and it could be subject to fine-grained thread-safe accesses among any threads running in racing requests. In this case, access should be denied. > If the flag belongs to the class, it means that for every function call it is > necessary to access the class of this. Do we want such check? We already have it: http://lxr.mozilla.org/mozilla/source/js/src/jsinterp.c#530 So JSCLASS_SINGLE_THREADED, not SCOPE_SINGLE_THREADED. /be

It's a matter of definition, but JSCLASS_SINGLE_REQUEST might be a better name. /be

(In reply to comment #10) > > But what about > > someOtherGeneratorInstance.next.call(openedGeneratorOnAnotherThread) ? > > The object referenced by openedGeneratorOnAnotherThread will be no longer in > use by a request active on that other thread, or else will be shared among > threads with a thin lock instead of the ownercx flyweight lock. ... > > If the flag belongs to the class, it means that for every function call it is > > necessary to access the class of this. Do we want such check? > > We already have it: http://lxr.mozilla.org/mozilla/source/js/src/jsinterp.c#530 Thanks, I missed that. Just to get it right: since ComputeThis calls OBJ_GET_CLASS that calls ClaimScope to do the thread magic to claim the scope, it is necessary just to check that ownercx is not null for objects with JSCLASS_SINGLE_THREADED set? I.e. after OBJ_GET_CLASS either ownercx is null or ownercx == cx? Regards, Igor

(In reply to comment #12) > (In reply to comment #10) > > > > But what about > > > someOtherGeneratorInstance.next.call(openedGeneratorOnAnotherThread) ? > > > > The object referenced by openedGeneratorOnAnotherThread will be no longer in > > use by a request active on that other thread, or else will be shared among > > threads with a thin lock instead of the ownercx flyweight lock. BTW, AOL made me patent the request-amortized flyweight object locking invention, but the MPL's patent clause grants everyone in the world right to use it in its SpiderMonkey implementation. > > > If the flag belongs to the class, it means that for every function call it is > > > necessary to access the class of this. Do we want such check? > > > > We already have it: http://lxr.mozilla.org/mozilla/source/js/src/jsinterp.c#530 > > Thanks, I missed that. > > Just to get it right: since ComputeThis calls OBJ_GET_CLASS that calls > ClaimScope to do the thread magic to claim the scope, it is necessary just to > check that ownercx is not null for objects with JSCLASS_SINGLE_THREADED set? > I.e. after OBJ_GET_CLASS either ownercx is null or ownercx == cx? Yes. /be

Here is universal JSCLASS_SINGLE_THREADED flag approach. The patch marks iterator, generator and XML classes with the new flag and enforces the restriction in js_ComputeThis. But to protect XML it is not enough. XML objects exposes their state through properties, so for them more has to be done.

Morphing the title to better reflect a generic nature of the problem the latest patch tries to address.

I'll review this week. /be

Igor, can you refresh this patch easily? If not I will do it. I should have reviewed it a while ago, obviously -- sorry about that! /be

Stealing, making a blocker for array thread safety. /be

Quick poke here. How are we doing on this, Brendan?

I'm behind on this. We could try to get away with it and see how that goes. This bug blocks js1.8src release. /be

Does "get away with it" mean land Igor's "Implementation v3" patch? or does that mean slip this beyond 1.9?

Brendan, think we should do this in a dot release? This seems scary.

Yeah, sorry -- comment 20 meant to say that. /be

Releasing this bug -- I haven't been working on it.

I think I actually fixed this.

Don't see how, but we could WONTFIX this, or make it depend on the MT proxies bug (bug 558866 IIRC). /be

Maybe I misunderstand the bug but now sharing an iterator no longer misbehaves any more than executing unsynchronized JS code outside an iterator. If you pull from an iterator from 2 threads, you get always valid js values, nothing crashes. Your program will behave weird and erratically, but thats really a high level application error. Am I missing something here?

Brendan, what do we want to do with this bug?

Native objects will be ST, so they become MT proxies if desirable/possible/easy, else throw. Case by case, we could make different arguments, but it would be fine to start by making these classes throw rather than "become". We need some code to do that, though, so there's a patch wanted here. /be

Solved by single-threaded runtime.