bugKawa - Bugs: bug #33325, using setIndirectDefines safely

 
 

bug #33325: using setIndirectDefines safely

Submitted by:  tk <huh>
Submitted on:  Mon 16 May 2011 04:44:15 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Item Group: Unexpected resultStatus: Need Info
Privacy: PublicAssigned to: Per Bothner <bothner>
Open/Closed: Open

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

Tue 17 May 2011 12:57:40 AM UTC, comment #2:

the problem case is Environment.cloneForThread, where
InheritingEnvironments are created in the context of
new threads (which may happen outside of application control).

it is possible to call setIndirectDefines in application code, but
it's tedious and error prone in situations where one is passing
in callback objects that are run in the context of a thread pool
(e..g. i'm using kawa with the netty async i/o framework).
To ensure that setIndirectDefines is seen early enough, it must
be called on every potential execution path.

i agree that the proposed change feels more like a workaround
than an actual solution -- i'll give some thought to better
ways of expressing this.

tk <huh>
Mon 16 May 2011 07:52:55 PM UTC, comment #1:

This does not seem like the right solution. Setting a static flag to control behavior usually isn't. (I know there are a number of these in Kawa, but I don't like it, and in any case most of these are set as defaults from the Kawa command line, where setting global state might be tolerable.)

So the obvious question is: Why is setIndirectDefines called too late? Would it be possible to call Environment.make (followed by Environment.setCurrent) explicitly and early when the thread is created? Or perhaps add a new Environment.makeShared factory method?

If we really need a global flag, it might be cleaner to use a gnu.mapping.ThreadLocation (which is what Scheme's make-parameter is built on).

Per Bothner <bothner>
Project AdministratorIn charge of this item.
Mon 16 May 2011 04:44:15 PM UTC, original submission:

I'd like to suggest the following patch to allow one to make
setting INDIRECT_DEFINES the default for new InheritingEnvironments.
It simply adds a static flag to gnu.mapping.InheritingEnvironment
to specify that setIndirectDefines be called in the constructor.

The rationale is that setting INDIRECT_DEFINES can only safely be
done at the time the environment is created. Attempting to set this
flag later, in application code, can put the environment into an
inconsistent state (because of the way Kawa uses timestamps for
SharedLocations). For implicitly created environments (e.g. in futures),
the constructor is the safe place to do this.

I don't think I've thoroughly understood how Kawa uses these timestamps
for lookups, but this change takes care of a problem where symbol
lookups were failing in some circumstances where setIndirectDefines is
called too late.

Index: gnu/mapping/ChangeLog
===================================================================
--- gnu/mapping/ChangeLog (revision 6981)
+++ gnu/mapping/ChangeLog (working copy)
@@ -1,3 +1,9 @@
+2011-05-16 thomas kirk <tk@research.att.com>
+
+ * InheritingEnvironment.java (DEFAULT_INDIRECT_DEFINES): New flag.
+ (constructor): call setIndirectDefines upon creation of environment
+ if DEFAULT_INDIRECT_DEFINES is true.
+
2011-04-25 Charles Turner <chturne@gmail.com>
Per Bothner <per@bothner.com>

Index: gnu/mapping/InheritingEnvironment.java
===================================================================
--- gnu/mapping/InheritingEnvironment.java (revision 6981)
+++ gnu/mapping/InheritingEnvironment.java (working copy)
@@ -9,6 +9,10 @@
Environment[] inherited;
int baseTimestamp;

+ /* true if newly created InheritingEnvironments should
+ set INDIRECT_DEFINES by default */
+ public static boolean DEFAULT_INDIRECT_DEFINES = false;
+
public InheritingEnvironment (String name, Environment parent)
{
super(name);
@@ -19,6 +23,9 @@
baseTimestamp = timestamp;
currentTimestamp = timestamp;
}
+
+ if (DEFAULT_INDIRECT_DEFINES)
+ setIndirectDefines();
}

public final int getNumParents () { return numInherited; }

tk <huh>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by bothner (Posted a comment)
  • -unavailable- added by huh (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 2 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 16 May 2011 07:52:55 PM UTCbothnerStatusNone=>Need Info
      Assigned toNone=>bothner

    Back to the top


    Powered by Savane 3.1-cleanup