bugKawa - Bugs: bug #33325, using setIndirectDefines safely

 
 

bug #33325: using setIndirectDefines safely

Submitter:  tk <huh>
Submitted:  Mon 16 May 2011 04:44:15 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  Unexpected result Status:  Need Info
Privacy:  Public Assigned to:  bothner
Open/Closed:  Open
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group administrator
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 Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-05-16 bothner StatusNone Need Info
        Assigned toNone bothner

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code