Last Comment Bug 349263 - Make iterators and generators thread-safe, but not necessarily MT
: Make iterators and generators thread-safe, but not necessarily MT
Status: RESOLVED WORKSFORME
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P2 normal with 2 votes (vote)
: ---
Assigned To: Andreas Gal :gal
:
Mentors:
Depends on:
Blocks: geniter js1.7src 419537
  Show dependency treegraph
 
Reported: 2006-08-18 17:32 PDT by Brendan Eich [:brendan]
Modified: 2014-01-03 10:06 PST (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implementation (3.57 KB, patch)
2006-08-19 08:05 PDT, Igor Bukanov
no flags Details | Diff | Review
Implementation v2 (4.29 KB, patch)
2006-08-30 03:08 PDT, Igor Bukanov
no flags Details | Diff | Review
Implementation v3 (15.59 KB, patch)
2006-08-31 02:13 PDT, Igor Bukanov
no flags Details | Diff | Review

Description Brendan Eich [:brendan] 2006-08-18 17:32:09 PDT
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
Comment 1 Igor Bukanov 2006-08-18 18:00:01 PDT
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.
Comment 2 Igor Bukanov 2006-08-19 08:05:39 PDT
Created attachment 234589 [details] [diff] [review]
Implementation

Here is a trivial implementation of creator-thread-only idea build on top of the patch from bug 349320.
Comment 3 Brendan Eich [:brendan] 2006-08-25 12:36:15 PDT
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
Comment 4 Igor Bukanov 2006-08-25 14:14:47 PDT
(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. 
Comment 5 Brendan Eich [:brendan] 2006-08-25 14:27:39 PDT
(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
Comment 6 Brendan Eich [:brendan] 2006-08-25 14:33:21 PDT
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
Comment 7 Igor Bukanov 2006-08-30 03:08:27 PDT
Created attachment 236058 [details] [diff] [review]
Implementation v2

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 8 Brendan Eich [:brendan] 2006-08-30 09:47:44 PDT
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
Comment 9 Igor Bukanov 2006-08-30 12:01:24 PDT
(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?
Comment 10 Brendan Eich [:brendan] 2006-08-30 13:41:31 PDT
(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
Comment 11 Brendan Eich [:brendan] 2006-08-30 13:43:27 PDT
It's a matter of definition, but JSCLASS_SINGLE_REQUEST might be a better name.

/be
Comment 12 Igor Bukanov 2006-08-30 14:21:25 PDT
(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
Comment 13 Brendan Eich [:brendan] 2006-08-30 15:43:26 PDT
(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
Comment 14 Igor Bukanov 2006-08-31 02:13:45 PDT
Created attachment 236204 [details] [diff] [review]
Implementation v3

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.
Comment 15 Igor Bukanov 2006-08-31 02:15:55 PDT
Morphing the title to better reflect a generic nature of the problem the latest patch tries to address.
Comment 16 Brendan Eich [:brendan] 2006-10-01 23:33:42 PDT
I'll review this week.

/be
Comment 17 Brendan Eich [:brendan] 2007-11-04 02:02:56 PST
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
Comment 18 Brendan Eich [:brendan] 2008-02-28 16:28:34 PST
Stealing, making a blocker for array thread safety.

/be
Comment 19 Damon Sicore (:damons) 2008-03-13 11:46:54 PDT
Quick poke here.  How are we doing on this, Brendan?  
Comment 20 Brendan Eich [:brendan] 2008-04-11 15:33:57 PDT
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
Comment 21 Brian Crowder 2008-04-14 14:08:14 PDT
Does "get away with it" mean land Igor's "Implementation v3" patch?  or does that mean slip this beyond 1.9?
Comment 22 Damon Sicore (:damons) 2008-04-21 12:03:38 PDT
Brendan, think we should do this in a dot release?  This seems scary.
Comment 23 Brendan Eich [:brendan] 2008-04-21 16:58:07 PDT
Yeah, sorry -- comment 20 meant to say that.

/be
Comment 24 Jason Orendorff [:jorendorff] 2010-04-26 15:29:42 PDT
Releasing this bug -- I haven't been working on it.
Comment 25 Andreas Gal :gal 2010-04-26 16:00:23 PDT
I think I actually fixed this.
Comment 26 Brendan Eich [:brendan] 2010-04-26 16:07:06 PDT
Don't see how, but we could WONTFIX this, or make it depend on the MT proxies bug (bug 558866 IIRC).

/be
Comment 27 Andreas Gal :gal 2010-04-26 16:14:49 PDT
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?
Comment 28 Andreas Gal :gal 2010-05-25 14:34:58 PDT
Brendan, what do we want to do with this bug?
Comment 29 Brendan Eich [:brendan] 2010-05-25 15:20:44 PDT
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
Comment 30 Luke Wagner [:luke] 2014-01-03 10:06:24 PST
Solved by single-threaded runtime.

Note You need to log in before you can comment on or make changes to this bug.