After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 558741 - Mutual imports cause great confusion
Mutual imports cause great confusion
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gjs-maint
gjs-maint
Depends on:
Blocks:
 
 
Reported: 2008-10-31 22:30 UTC by Owen Taylor
Modified: 2008-11-09 17:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch adding failing test (1.83 KB, patch)
2008-10-31 22:34 UTC, Owen Taylor
none Details | Review
Patch fixing problem (8.67 KB, patch)
2008-11-09 03:36 UTC, Owen Taylor
none Details | Review

Description Owen Taylor 2008-10-31 22:30:07 UTC
If module A imports module B then module B imports module A, what happens is:

 - module A is loaded once
 - module B is loaded
 - module A is loaded a second time
 - the second copy of module A is stored in imports.a
 - module B is stored in imports.b
 - the first copy of module A is stored in imports.a

Other modules may be using either the first or second copy of module.A depending on when they were loaded, and in general things go to hell in very unpredictable ways.

On import start, the module object needs to be immediately stored in imports.a 
so that other modules see it is there.

On failure, ideally all assignments to imports.* done during the import would be rolled back. Rolling back just the assignment to imports.a could leave things in an inconsistent state, since other modules might reference the dictionary. However, I think getting that right is low priority... it's much less of a pitfall if an exception during a complicated import causes a
inconsistent state, then if a succesful import causes an inconsistent state.

I'll attach a test case.
Comment 1 Owen Taylor 2008-10-31 22:34:09 UTC
Created attachment 121759 [details] [review]
Patch adding failing test
Comment 2 Havoc Pennington 2008-10-31 22:42:15 UTC
Missing "const" in "B = imports.mutualImport.b"

Otherwise test looks good (but should not commit until it passes)
Comment 3 Owen Taylor 2008-11-09 03:36:02 UTC
Created attachment 122250 [details] [review]
Patch fixing problem

Here's a patch. It only backs out the immediately failing import
and not all imports in the operation (there's a comment in the
patch explaining the situation in case anybody runs into corner cases
and wants to try and improve.)

The dance I do to not regress and set properties with 
JSPROP_PERMANENT is a bit ridiculous - set the property, do the import,
set the property again with JSPROP_PERMANENT. Not sure there's a lot
of value there.
Comment 4 Havoc Pennington 2008-11-09 17:38:50 UTC
Patch looks good to me
Comment 5 Owen Taylor 2008-11-09 17:59:00 UTC
Committed.