RRWL with 'bad' Thread.getId() implementations

classic Classic list List threaded Threaded
33 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RRWL with 'bad' Thread.getId() implementations

Chris Dennis
Hi All,

While dealing with a customer issue, I ran in to the possibility of
breaking a RRWL by feeding in Thread instances with colliding thread-ids.
Inside RRWL the cachedHoldCounter logic assumes that getId() will return a
unique identifier for a thread.  Do people consider this a bug in RRWL or
not? (I think it would be agreed that this is also a bug in the
subclassing of Thread)

Regards,

Chris Dennis

public static void main(String[] args) {
  final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
  final CyclicBarrier barrier = new CyclicBarrier(2);
   
  Thread t1 = new EvilThread() {
    public void run() {
      try {
        lock.readLock().lock();
        barrier.await();
        //T2 locks
        barrier.await();
        //T3 locks
        //T3 unlocks
        barrier.await();
        //T2 unlocks
        barrier.await();
        lock.readLock().unlock();
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
     
  };
  Thread t2 = new EvilThread() {
    public void run() {
      try {
        //T1 locks
                  barrier.await();
                  lock.readLock().lock();
                  barrier.await();
                  //T3 locks
                  //T3 unlocks
                  barrier.await();
                  lock.readLock().unlock();
                  barrier.await();
                  //T1 unlocks
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
     
  };
  Thread t3 = new EvilThread() {
    public void run() {
      try {
        //T1 locks
        barrier.await();
                  //T2 locks
                  barrier.await();
                  lock.readLock().lock();
                  lock.readLock().unlock();
                  barrier.await();
                  //T2 unlocks
                  barrier.await();
                  //T3 unlocks
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
     
  };
   
  t1.start();
  t2.start();
  t3.start();
  }

 
  static class EvilThread extends Thread {
  public long getId() {
    return 42L;
  }
  }


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: RRWL with 'bad' Thread.getId() implementations

Nathan Reynolds-2
The JavaDoc for Thread.getId() says "...thread ID is unique..." so I don't think this is a bug in RRWL.  Maybe Thread.getId() should be declared final.

We might want to consider going as far as declaring the member field "tid" as final.  This could be done via "private long tid = nextThreadID();"

I find it very interesting that threadInitNumber and threadSeqNumber are both used in the Thread class.  It seems we only need 1.  It seems that the constructor should use "Thread-" + tid for a thread name.  In fact, the name could read "Thread-10" and the tid could be 7 because there is a race between when the name is generated and the tid is set.  The mismatch probably doesn't matter functionally.  However, it could make it easier for debugging.

Nathan Reynolds | Architect | 602.333.9091
Oracle PSR Engineering | Server Technology
On 6/25/2013 9:50 AM, Chris Dennis wrote:
Hi All,

While dealing with a customer issue, I ran in to the possibility of
breaking a RRWL by feeding in Thread instances with colliding thread-ids.
Inside RRWL the cachedHoldCounter logic assumes that getId() will return a
unique identifier for a thread.  Do people consider this a bug in RRWL or
not? (I think it would be agreed that this is also a bug in the
subclassing of Thread)

Regards,

Chris Dennis

public static void main(String[] args) {
  final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
  final CyclicBarrier barrier = new CyclicBarrier(2);
    
  Thread t1 = new EvilThread() {
    public void run() {
      try {
        lock.readLock().lock();
        barrier.await();
        //T2 locks
        barrier.await();
        //T3 locks
        //T3 unlocks
        barrier.await();
        //T2 unlocks
        barrier.await();
        lock.readLock().unlock();
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
      
  };
  Thread t2 = new EvilThread() {
    public void run() {
      try {
        //T1 locks
                  barrier.await();
                  lock.readLock().lock();
                  barrier.await();
                  //T3 locks
                  //T3 unlocks
                  barrier.await();
                  lock.readLock().unlock();
                  barrier.await();
                  //T1 unlocks
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
      
  };
  Thread t3 = new EvilThread() {
    public void run() {
      try {
        //T1 locks
        barrier.await();
                  //T2 locks
                  barrier.await();
                  lock.readLock().lock();
                  lock.readLock().unlock();
                  barrier.await();
                  //T2 unlocks
                  barrier.await();
                  //T3 unlocks
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
      
  };
    
  t1.start();
  t2.start();
  t3.start();
  }

  
  static class EvilThread extends Thread {
  public long getId() {
    return 42L;
  }
  }


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest




_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: RRWL with 'bad' Thread.getId() implementations

Chris Dennis
The issue about getId() not being final has been thrown about forever – it's not going to be fixed because it would cause too much breakage in user code.  Seems strange to me though given that we know the non finality of getId allows users to do this, we aren't avoiding relying on that method – why don't we have some other way for library code (JDK or third-party) to get access to a unique identifier for a thread?

On 6/25/13 1:24 PM, "Nathan Reynolds" <[hidden email]> wrote:

The JavaDoc for Thread.getId() says "...thread ID is unique..." so I don't think this is a bug in RRWL.  Maybe Thread.getId() should be declared final.

We might want to consider going as far as declaring the member field "tid" as final.  This could be done via "private long tid = nextThreadID();"

I find it very interesting that threadInitNumber and threadSeqNumber are both used in the Thread class.  It seems we only need 1.  It seems that the constructor should use "Thread-" + tid for a thread name.  In fact, the name could read "Thread-10" and the tid could be 7 because there is a race between when the name is generated and the tid is set.  The mismatch probably doesn't matter functionally.  However, it could make it easier for debugging.

Nathan Reynolds | Architect | 602.333.9091
Oracle PSR Engineering | Server Technology
On 6/25/2013 9:50 AM, Chris Dennis wrote:
Hi All,

While dealing with a customer issue, I ran in to the possibility of
breaking a RRWL by feeding in Thread instances with colliding thread-ids.
Inside RRWL the cachedHoldCounter logic assumes that getId() will return a
unique identifier for a thread.  Do people consider this a bug in RRWL or
not? (I think it would be agreed that this is also a bug in the
subclassing of Thread)

Regards,

Chris Dennis

public static void main(String[] args) {
  final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
  final CyclicBarrier barrier = new CyclicBarrier(2);

  Thread t1 = new EvilThread() {
    public void run() {
      try {
        lock.readLock().lock();
        barrier.await();
        //T2 locks
        barrier.await();
        //T3 locks
        //T3 unlocks
        barrier.await();
        //T2 unlocks
        barrier.await();
        lock.readLock().unlock();
      } catch (Exception e) {
        e.printStackTrace();
      }
    }

  };
  Thread t2 = new EvilThread() {
    public void run() {
      try {
        //T1 locks
                  barrier.await();
                  lock.readLock().lock();
                  barrier.await();
                  //T3 locks
                  //T3 unlocks
                  barrier.await();
                  lock.readLock().unlock();
                  barrier.await();
                  //T1 unlocks
      } catch (Exception e) {
        e.printStackTrace();
      }
    }

  };
  Thread t3 = new EvilThread() {
    public void run() {
      try {
        //T1 locks
        barrier.await();
                  //T2 locks
                  barrier.await();
                  lock.readLock().lock();
                  lock.readLock().unlock();
                  barrier.await();
                  //T2 unlocks
                  barrier.await();
                  //T3 unlocks
      } catch (Exception e) {
        e.printStackTrace();
      }
    }

  };

  t1.start();
  t2.start();
  t3.start();
  }


  static class EvilThread extends Thread {
  public long getId() {
    return 42L;
  }
  }


_______________________________________________
Concurrency-interest mailing list
[hidden email]http://cs.oswego.edu/mailman/listinfo/concurrency-interest

_______________________________________________ Concurrency-interest mailing list [hidden email] http://cs.oswego.edu/mailman/listinfo/concurrency-interest

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: RRWL with 'bad' Thread.getId() implementations

Nathan Reynolds-2
Wow!  Final getId() would break user code?  Interesting.  Are there any examples?  I can dream up a few but I am curious what is out in the wild.

I guess we need another method in Thread which is declared final and returns the "tid" member variable value.  Maybe it could be called getFinalId(), getImmutableId(), getFirmId(), or getStableId().

Nathan Reynolds | Architect | 602.333.9091
Oracle PSR Engineering | Server Technology
On 6/25/2013 10:56 AM, Chris Dennis wrote:
The issue about getId() not being final has been thrown about forever – it's not going to be fixed because it would cause too much breakage in user code.  Seems strange to me though given that we know the non finality of getId allows users to do this, we aren't avoiding relying on that method – why don't we have some other way for library code (JDK or third-party) to get access to a unique identifier for a thread?

On 6/25/13 1:24 PM, "Nathan Reynolds" <[hidden email]> wrote:

The JavaDoc for Thread.getId() says "...thread ID is unique..." so I don't think this is a bug in RRWL.  Maybe Thread.getId() should be declared final.

We might want to consider going as far as declaring the member field "tid" as final.  This could be done via "private long tid = nextThreadID();"

I find it very interesting that threadInitNumber and threadSeqNumber are both used in the Thread class.  It seems we only need 1.  It seems that the constructor should use "Thread-" + tid for a thread name.  In fact, the name could read "Thread-10" and the tid could be 7 because there is a race between when the name is generated and the tid is set.  The mismatch probably doesn't matter functionally.  However, it could make it easier for debugging.

Nathan Reynolds | Architect | 602.333.9091
Oracle PSR Engineering | Server Technology
On 6/25/2013 9:50 AM, Chris Dennis wrote:
Hi All,

While dealing with a customer issue, I ran in to the possibility of
breaking a RRWL by feeding in Thread instances with colliding thread-ids.
Inside RRWL the cachedHoldCounter logic assumes that getId() will return a
unique identifier for a thread.  Do people consider this a bug in RRWL or
not? (I think it would be agreed that this is also a bug in the
subclassing of Thread)

Regards,

Chris Dennis

public static void main(String[] args) {
  final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
  final CyclicBarrier barrier = new CyclicBarrier(2);
    
  Thread t1 = new EvilThread() {
    public void run() {
      try {
        lock.readLock().lock();
        barrier.await();
        //T2 locks
        barrier.await();
        //T3 locks
        //T3 unlocks
        barrier.await();
        //T2 unlocks
        barrier.await();
        lock.readLock().unlock();
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
      
  };
  Thread t2 = new EvilThread() {
    public void run() {
      try {
        //T1 locks
                  barrier.await();
                  lock.readLock().lock();
                  barrier.await();
                  //T3 locks
                  //T3 unlocks
                  barrier.await();
                  lock.readLock().unlock();
                  barrier.await();
                  //T1 unlocks
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
      
  };
  Thread t3 = new EvilThread() {
    public void run() {
      try {
        //T1 locks
        barrier.await();
                  //T2 locks
                  barrier.await();
                  lock.readLock().lock();
                  lock.readLock().unlock();
                  barrier.await();
                  //T2 unlocks
                  barrier.await();
                  //T3 unlocks
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
      
  };
    
  t1.start();
  t2.start();
  t3.start();
  }

  
  static class EvilThread extends Thread {
  public long getId() {
    return 42L;
  }
  }


_______________________________________________
Concurrency-interest mailing list
[hidden email]http://cs.oswego.edu/mailman/listinfo/concurrency-interest

_______________________________________________ Concurrency-interest mailing list [hidden email] http://cs.oswego.edu/mailman/listinfo/concurrency-interest


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: RRWL with 'bad' Thread.getId() implementations

Viktor Klang

Yeah, deprecate getId and introduce new final method sounds like a plan.

On Jun 25, 2013 8:08 PM, "Nathan Reynolds" <[hidden email]> wrote:
Wow!  Final getId() would break user code?  Interesting.  Are there any examples?  I can dream up a few but I am curious what is out in the wild.

I guess we need another method in Thread which is declared final and returns the "tid" member variable value.  Maybe it could be called getFinalId(), getImmutableId(), getFirmId(), or getStableId().

Nathan Reynolds | Architect | <a href="tel:602.333.9091" value="+16023339091" target="_blank">602.333.9091
Oracle PSR Engineering | Server Technology
On 6/25/2013 10:56 AM, Chris Dennis wrote:
The issue about getId() not being final has been thrown about forever – it's not going to be fixed because it would cause too much breakage in user code.  Seems strange to me though given that we know the non finality of getId allows users to do this, we aren't avoiding relying on that method – why don't we have some other way for library code (JDK or third-party) to get access to a unique identifier for a thread?

On 6/25/13 1:24 PM, "Nathan Reynolds" <[hidden email]> wrote:

The JavaDoc for Thread.getId() says "...thread ID is unique..." so I don't think this is a bug in RRWL.  Maybe Thread.getId() should be declared final.

We might want to consider going as far as declaring the member field "tid" as final.  This could be done via "private long tid = nextThreadID();"

I find it very interesting that threadInitNumber and threadSeqNumber are both used in the Thread class.  It seems we only need 1.  It seems that the constructor should use "Thread-" + tid for a thread name.  In fact, the name could read "Thread-10" and the tid could be 7 because there is a race between when the name is generated and the tid is set.  The mismatch probably doesn't matter functionally.  However, it could make it easier for debugging.

Nathan Reynolds | Architect | <a href="tel:602.333.9091" value="+16023339091" target="_blank">602.333.9091
Oracle PSR Engineering | Server Technology
On 6/25/2013 9:50 AM, Chris Dennis wrote:
Hi All,

While dealing with a customer issue, I ran in to the possibility of
breaking a RRWL by feeding in Thread instances with colliding thread-ids.
Inside RRWL the cachedHoldCounter logic assumes that getId() will return a
unique identifier for a thread.  Do people consider this a bug in RRWL or
not? (I think it would be agreed that this is also a bug in the
subclassing of Thread)

Regards,

Chris Dennis

public static void main(String[] args) {
  final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
  final CyclicBarrier barrier = new CyclicBarrier(2);
    
  Thread t1 = new EvilThread() {
    public void run() {
      try {
        lock.readLock().lock();
        barrier.await();
        //T2 locks
        barrier.await();
        //T3 locks
        //T3 unlocks
        barrier.await();
        //T2 unlocks
        barrier.await();
        lock.readLock().unlock();
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
      
  };
  Thread t2 = new EvilThread() {
    public void run() {
      try {
        //T1 locks
                  barrier.await();
                  lock.readLock().lock();
                  barrier.await();
                  //T3 locks
                  //T3 unlocks
                  barrier.await();
                  lock.readLock().unlock();
                  barrier.await();
                  //T1 unlocks
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
      
  };
  Thread t3 = new EvilThread() {
    public void run() {
      try {
        //T1 locks
        barrier.await();
                  //T2 locks
                  barrier.await();
                  lock.readLock().lock();
                  lock.readLock().unlock();
                  barrier.await();
                  //T2 unlocks
                  barrier.await();
                  //T3 unlocks
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
      
  };
    
  t1.start();
  t2.start();
  t3.start();
  }

  
  static class EvilThread extends Thread {
  public long getId() {
    return 42L;
  }
  }


_______________________________________________
Concurrency-interest mailing list
[hidden email]http://cs.oswego.edu/mailman/listinfo/concurrency-interest

_______________________________________________ Concurrency-interest mailing list [hidden email] http://cs.oswego.edu/mailman/listinfo/concurrency-interest


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: RRWL with 'bad' Thread.getId() implementations

Chris Dennis
In reply to this post by Nathan Reynolds-2
Yes, sadly people will insist on hanging themselves whenever we give them enough rope.  I don't think I've actually seen someones code that did this, but I have seen it's fallout: http://forums.terracotta.org/forums/posts/list/0/2116.page

Something like getStableId() would seem like a good idea, although I strongly suspect this debate has occurred within Sun/Oracle and there are probably good technical reasons why it's not happened yet.  What I would like to see as a minimum is someone adding some stronger wording to the Javadoc for the method.

Chris

On 6/25/13 2:02 PM, "Nathan Reynolds" <[hidden email]> wrote:

Wow!  Final getId() would break user code?  Interesting.  Are there any examples?  I can dream up a few but I am curious what is out in the wild.

I guess we need another method in Thread which is declared final and returns the "tid" member variable value.  Maybe it could be called getFinalId(), getImmutableId(), getFirmId(), or getStableId().

Nathan Reynolds | Architect | 602.333.9091
Oracle PSR Engineering | Server Technology
On 6/25/2013 10:56 AM, Chris Dennis wrote:
The issue about getId() not being final has been thrown about forever – it's not going to be fixed because it would cause too much breakage in user code.  Seems strange to me though given that we know the non finality of getId allows users to do this, we aren't avoiding relying on that method – why don't we have some other way for library code (JDK or third-party) to get access to a unique identifier for a thread?

On 6/25/13 1:24 PM, "Nathan Reynolds" <[hidden email]> wrote:

The JavaDoc for Thread.getId() says "...thread ID is unique..." so I don't think this is a bug in RRWL.  Maybe Thread.getId() should be declared final.

We might want to consider going as far as declaring the member field "tid" as final.  This could be done via "private long tid = nextThreadID();"

I find it very interesting that threadInitNumber and threadSeqNumber are both used in the Thread class.  It seems we only need 1.  It seems that the constructor should use "Thread-" + tid for a thread name.  In fact, the name could read "Thread-10" and the tid could be 7 because there is a race between when the name is generated and the tid is set.  The mismatch probably doesn't matter functionally.  However, it could make it easier for debugging.

Nathan Reynolds | Architect | 602.333.9091
Oracle PSR Engineering | Server Technology
On 6/25/2013 9:50 AM, Chris Dennis wrote:
Hi All,

While dealing with a customer issue, I ran in to the possibility of
breaking a RRWL by feeding in Thread instances with colliding thread-ids.
Inside RRWL the cachedHoldCounter logic assumes that getId() will return a
unique identifier for a thread.  Do people consider this a bug in RRWL or
not? (I think it would be agreed that this is also a bug in the
subclassing of Thread)

Regards,

Chris Dennis

public static void main(String[] args) {
  final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
  final CyclicBarrier barrier = new CyclicBarrier(2);

  Thread t1 = new EvilThread() {
    public void run() {
      try {
        lock.readLock().lock();
        barrier.await();
        //T2 locks
        barrier.await();
        //T3 locks
        //T3 unlocks
        barrier.await();
        //T2 unlocks
        barrier.await();
        lock.readLock().unlock();
      } catch (Exception e) {
        e.printStackTrace();
      }
    }

  };
  Thread t2 = new EvilThread() {
    public void run() {
      try {
        //T1 locks
                  barrier.await();
                  lock.readLock().lock();
                  barrier.await();
                  //T3 locks
                  //T3 unlocks
                  barrier.await();
                  lock.readLock().unlock();
                  barrier.await();
                  //T1 unlocks
      } catch (Exception e) {
        e.printStackTrace();
      }
    }

  };
  Thread t3 = new EvilThread() {
    public void run() {
      try {
        //T1 locks
        barrier.await();
                  //T2 locks
                  barrier.await();
                  lock.readLock().lock();
                  lock.readLock().unlock();
                  barrier.await();
                  //T2 unlocks
                  barrier.await();
                  //T3 unlocks
      } catch (Exception e) {
        e.printStackTrace();
      }
    }

  };

  t1.start();
  t2.start();
  t3.start();
  }


  static class EvilThread extends Thread {
  public long getId() {
    return 42L;
  }
  }


_______________________________________________
Concurrency-interest mailing list
[hidden email]http://cs.oswego.edu/mailman/listinfo/concurrency-interest

_______________________________________________ Concurrency-interest mailing list [hidden email] http://cs.oswego.edu/mailman/listinfo/concurrency-interest


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: RRWL with 'bad' Thread.getId() implementations

Nathan Reynolds-2
I have filed bug https://jbs.oracle.com/bugs/browse/JDK-8017617 for this.

Nathan Reynolds | Architect | 602.333.9091
Oracle PSR Engineering | Server Technology
On 6/25/2013 11:30 AM, Chris Dennis wrote:
Yes, sadly people will insist on hanging themselves whenever we give them enough rope.  I don't think I've actually seen someones code that did this, but I have seen it's fallout: http://forums.terracotta.org/forums/posts/list/0/2116.page

Something like getStableId() would seem like a good idea, although I strongly suspect this debate has occurred within Sun/Oracle and there are probably good technical reasons why it's not happened yet.  What I would like to see as a minimum is someone adding some stronger wording to the Javadoc for the method.

Chris

On 6/25/13 2:02 PM, "Nathan Reynolds" <[hidden email]> wrote:

Wow!  Final getId() would break user code?  Interesting.  Are there any examples?  I can dream up a few but I am curious what is out in the wild.

I guess we need another method in Thread which is declared final and returns the "tid" member variable value.  Maybe it could be called getFinalId(), getImmutableId(), getFirmId(), or getStableId().

Nathan Reynolds | Architect | 602.333.9091
Oracle PSR Engineering | Server Technology
On 6/25/2013 10:56 AM, Chris Dennis wrote:
The issue about getId() not being final has been thrown about forever – it's not going to be fixed because it would cause too much breakage in user code.  Seems strange to me though given that we know the non finality of getId allows users to do this, we aren't avoiding relying on that method – why don't we have some other way for library code (JDK or third-party) to get access to a unique identifier for a thread?

On 6/25/13 1:24 PM, "Nathan Reynolds" <[hidden email]> wrote:

The JavaDoc for Thread.getId() says "...thread ID is unique..." so I don't think this is a bug in RRWL.  Maybe Thread.getId() should be declared final.

We might want to consider going as far as declaring the member field "tid" as final.  This could be done via "private long tid = nextThreadID();"

I find it very interesting that threadInitNumber and threadSeqNumber are both used in the Thread class.  It seems we only need 1.  It seems that the constructor should use "Thread-" + tid for a thread name.  In fact, the name could read "Thread-10" and the tid could be 7 because there is a race between when the name is generated and the tid is set.  The mismatch probably doesn't matter functionally.  However, it could make it easier for debugging.

Nathan Reynolds | Architect | 602.333.9091
Oracle PSR Engineering | Server Technology
On 6/25/2013 9:50 AM, Chris Dennis wrote:
Hi All,

While dealing with a customer issue, I ran in to the possibility of
breaking a RRWL by feeding in Thread instances with colliding thread-ids.
Inside RRWL the cachedHoldCounter logic assumes that getId() will return a
unique identifier for a thread.  Do people consider this a bug in RRWL or
not? (I think it would be agreed that this is also a bug in the
subclassing of Thread)

Regards,

Chris Dennis

public static void main(String[] args) {
  final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
  final CyclicBarrier barrier = new CyclicBarrier(2);
    
  Thread t1 = new EvilThread() {
    public void run() {
      try {
        lock.readLock().lock();
        barrier.await();
        //T2 locks
        barrier.await();
        //T3 locks
        //T3 unlocks
        barrier.await();
        //T2 unlocks
        barrier.await();
        lock.readLock().unlock();
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
      
  };
  Thread t2 = new EvilThread() {
    public void run() {
      try {
        //T1 locks
                  barrier.await();
                  lock.readLock().lock();
                  barrier.await();
                  //T3 locks
                  //T3 unlocks
                  barrier.await();
                  lock.readLock().unlock();
                  barrier.await();
                  //T1 unlocks
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
      
  };
  Thread t3 = new EvilThread() {
    public void run() {
      try {
        //T1 locks
        barrier.await();
                  //T2 locks
                  barrier.await();
                  lock.readLock().lock();
                  lock.readLock().unlock();
                  barrier.await();
                  //T2 unlocks
                  barrier.await();
                  //T3 unlocks
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
      
  };
    
  t1.start();
  t2.start();
  t3.start();
  }

  
  static class EvilThread extends Thread {
  public long getId() {
    return 42L;
  }
  }


_______________________________________________
Concurrency-interest mailing list
[hidden email]http://cs.oswego.edu/mailman/listinfo/concurrency-interest

_______________________________________________ Concurrency-interest mailing list [hidden email] http://cs.oswego.edu/mailman/listinfo/concurrency-interest



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: RRWL with 'bad' Thread.getId() implementations

bestsss
I thought that Thread.getId() breaking RWLock was a known issue. That was one of the 1st observation I made years ago. The reliance on it was awkward and done in a rush, the way to verify uniqueness is via == and weak references, thread.id looked like a hash back then.

While getId() shall not be declared final it shall be a subject of the same inspection as set/getClassLoaderContext.

I have personally overridden Thread.getId() to get around some library restrictions about calling conventions. So it's way too late to be declared final.

Stanimir

On Tue, Jun 25, 2013 at 9:33 PM, Nathan Reynolds <[hidden email]> wrote:
I have filed bug https://jbs.oracle.com/bugs/browse/JDK-8017617 for this.
 
Nathan Reynolds | Architect | <a href="tel:602.333.9091" value="+16023339091" target="_blank">602.333.9091
Oracle PSR Engineering | Server Technology
On 6/25/2013 11:30 AM, Chris Dennis wrote:
Yes, sadly people will insist on hanging themselves whenever we give them enough rope.  I don't think I've actually seen someones code that did this, but I have seen it's fallout: http://forums.terracotta.org/forums/posts/list/0/2116.page

Something like getStableId() would seem like a good idea, although I strongly suspect this debate has occurred within Sun/Oracle and there are probably good technical reasons why it's not happened yet.  What I would like to see as a minimum is someone adding some stronger wording to the Javadoc for the method.

Chris

On 6/25/13 2:02 PM, "Nathan Reynolds" <[hidden email]> wrote:

Wow!  Final getId() would break user code?  Interesting.  Are there any examples?  I can dream up a few but I am curious what is out in the wild.

I guess we need another method in Thread which is declared final and returns the "tid" member variable value.  Maybe it could be called getFinalId(), getImmutableId(), getFirmId(), or getStableId().

Nathan Reynolds | Architect | <a href="tel:602.333.9091" value="+16023339091" target="_blank">602.333.9091
Oracle PSR Engineering | Server Technology
On 6/25/2013 10:56 AM, Chris Dennis wrote:
The issue about getId() not being final has been thrown about forever – it's not going to be fixed because it would cause too much breakage in user code.  Seems strange to me though given that we know the non finality of getId allows users to do this, we aren't avoiding relying on that method – why don't we have some other way for library code (JDK or third-party) to get access to a unique identifier for a thread?

On 6/25/13 1:24 PM, "Nathan Reynolds" <[hidden email]> wrote:

The JavaDoc for Thread.getId() says "...thread ID is unique..." so I don't think this is a bug in RRWL.  Maybe Thread.getId() should be declared final.

We might want to consider going as far as declaring the member field "tid" as final.  This could be done via "private long tid = nextThreadID();"

I find it very interesting that threadInitNumber and threadSeqNumber are both used in the Thread class.  It seems we only need 1.  It seems that the constructor should use "Thread-" + tid for a thread name.  In fact, the name could read "Thread-10" and the tid could be 7 because there is a race between when the name is generated and the tid is set.  The mismatch probably doesn't matter functionally.  However, it could make it easier for debugging.

Nathan Reynolds | Architect | <a href="tel:602.333.9091" value="+16023339091" target="_blank">602.333.9091
Oracle PSR Engineering | Server Technology
On 6/25/2013 9:50 AM, Chris Dennis wrote:
Hi All,

While dealing with a customer issue, I ran in to the possibility of
breaking a RRWL by feeding in Thread instances with colliding thread-ids.
Inside RRWL the cachedHoldCounter logic assumes that getId() will return a
unique identifier for a thread.  Do people consider this a bug in RRWL or
not? (I think it would be agreed that this is also a bug in the
subclassing of Thread)

Regards,

Chris Dennis

public static void main(String[] args) {
  final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
  final CyclicBarrier barrier = new CyclicBarrier(2);
    
  Thread t1 = new EvilThread() {
    public void run() {
      try {
        lock.readLock().lock();
        barrier.await();
        //T2 locks
        barrier.await();
        //T3 locks
        //T3 unlocks
        barrier.await();
        //T2 unlocks
        barrier.await();
        lock.readLock().unlock();
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
      
  };
  Thread t2 = new EvilThread() {
    public void run() {
      try {
        //T1 locks
                  barrier.await();
                  lock.readLock().lock();
                  barrier.await();
                  //T3 locks
                  //T3 unlocks
                  barrier.await();
                  lock.readLock().unlock();
                  barrier.await();
                  //T1 unlocks
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
      
  };
  Thread t3 = new EvilThread() {
    public void run() {
      try {
        //T1 locks
        barrier.await();
                  //T2 locks
                  barrier.await();
                  lock.readLock().lock();
                  lock.readLock().unlock();
                  barrier.await();
                  //T2 unlocks
                  barrier.await();
                  //T3 unlocks
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
      
  };
    
  t1.start();
  t2.start();
  t3.start();
  }

  
  static class EvilThread extends Thread {
  public long getId() {
    return 42L;
  }
  }


_______________________________________________
Concurrency-interest mailing list
[hidden email]http://cs.oswego.edu/mailman/listinfo/concurrency-interest

_______________________________________________ Concurrency-interest mailing list [hidden email] http://cs.oswego.edu/mailman/listinfo/concurrency-interest



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: RRWL with 'bad' Thread.getId() implementations

bestsss
In reply to this post by Nathan Reynolds-2
Speaking of modifying Thread class:
Adding "public static Thread[] getAllThreads" would a welcome change. The functionally is available already through getAllStackTraces().keySet() but it has the added penalty of fetching the stack traces which is a non-trivial penalty hit.

Stanimir

On Tue, Jun 25, 2013 at 9:33 PM, Nathan Reynolds <[hidden email]> wrote:
I have filed bug https://jbs.oracle.com/bugs/browse/JDK-8017617 for this.

Nathan Reynolds | Architect | <a href="tel:602.333.9091" value="+16023339091" target="_blank">602.333.9091
Oracle PSR Engineering | Server Technology
On 6/25/2013 11:30 AM, Chris Dennis wrote:
Yes, sadly people will insist on hanging themselves whenever we give them enough rope.  I don't think I've actually seen someones code that did this, but I have seen it's fallout: http://forums.terracotta.org/forums/posts/list/0/2116.page

Something like getStableId() would seem like a good idea, although I strongly suspect this debate has occurred within Sun/Oracle and there are probably good technical reasons why it's not happened yet.  What I would like to see as a minimum is someone adding some stronger wording to the Javadoc for the method.

Chris

On 6/25/13 2:02 PM, "Nathan Reynolds" <[hidden email]> wrote:

Wow!  Final getId() would break user code?  Interesting.  Are there any examples?  I can dream up a few but I am curious what is out in the wild.

I guess we need another method in Thread which is declared final and returns the "tid" member variable value.  Maybe it could be called getFinalId(), getImmutableId(), getFirmId(), or getStableId().

Nathan Reynolds | Architect | <a href="tel:602.333.9091" value="+16023339091" target="_blank">602.333.9091
Oracle PSR Engineering | Server Technology
On 6/25/2013 10:56 AM, Chris Dennis wrote:
The issue about getId() not being final has been thrown about forever – it's not going to be fixed because it would cause too much breakage in user code.  Seems strange to me though given that we know the non finality of getId allows users to do this, we aren't avoiding relying on that method – why don't we have some other way for library code (JDK or third-party) to get access to a unique identifier for a thread?

On 6/25/13 1:24 PM, "Nathan Reynolds" <[hidden email]> wrote:

The JavaDoc for Thread.getId() says "...thread ID is unique..." so I don't think this is a bug in RRWL.  Maybe Thread.getId() should be declared final.

We might want to consider going as far as declaring the member field "tid" as final.  This could be done via "private long tid = nextThreadID();"

I find it very interesting that threadInitNumber and threadSeqNumber are both used in the Thread class.  It seems we only need 1.  It seems that the constructor should use "Thread-" + tid for a thread name.  In fact, the name could read "Thread-10" and the tid could be 7 because there is a race between when the name is generated and the tid is set.  The mismatch probably doesn't matter functionally.  However, it could make it easier for debugging.

Nathan Reynolds | Architect | <a href="tel:602.333.9091" value="+16023339091" target="_blank">602.333.9091
Oracle PSR Engineering | Server Technology
On 6/25/2013 9:50 AM, Chris Dennis wrote:
Hi All,

While dealing with a customer issue, I ran in to the possibility of
breaking a RRWL by feeding in Thread instances with colliding thread-ids.
Inside RRWL the cachedHoldCounter logic assumes that getId() will return a
unique identifier for a thread.  Do people consider this a bug in RRWL or
not? (I think it would be agreed that this is also a bug in the
subclassing of Thread)

Regards,

Chris Dennis

public static void main(String[] args) {
  final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
  final CyclicBarrier barrier = new CyclicBarrier(2);
    
  Thread t1 = new EvilThread() {
    public void run() {
      try {
        lock.readLock().lock();
        barrier.await();
        //T2 locks
        barrier.await();
        //T3 locks
        //T3 unlocks
        barrier.await();
        //T2 unlocks
        barrier.await();
        lock.readLock().unlock();
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
      
  };
  Thread t2 = new EvilThread() {
    public void run() {
      try {
        //T1 locks
                  barrier.await();
                  lock.readLock().lock();
                  barrier.await();
                  //T3 locks
                  //T3 unlocks
                  barrier.await();
                  lock.readLock().unlock();
                  barrier.await();
                  //T1 unlocks
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
      
  };
  Thread t3 = new EvilThread() {
    public void run() {
      try {
        //T1 locks
        barrier.await();
                  //T2 locks
                  barrier.await();
                  lock.readLock().lock();
                  lock.readLock().unlock();
                  barrier.await();
                  //T2 unlocks
                  barrier.await();
                  //T3 unlocks
      } catch (Exception e) {
        e.printStackTrace();
      }
    }
      
  };
    
  t1.start();
  t2.start();
  t3.start();
  }

  
  static class EvilThread extends Thread {
  public long getId() {
    return 42L;
  }
  }


_______________________________________________
Concurrency-interest mailing list
[hidden email]http://cs.oswego.edu/mailman/listinfo/concurrency-interest

_______________________________________________ Concurrency-interest mailing list [hidden email] http://cs.oswego.edu/mailman/listinfo/concurrency-interest



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: RRWL with 'bad' Thread.getId() implementations

Chris Hegarty
In reply to this post by Nathan Reynolds-2
On 06/25/2013 07:33 PM, Nathan Reynolds wrote:
> I have filed bug https://jbs.oracle.com/bugs/browse/JDK-8017617 for this.

The public bug for this will available, soon, at:
   http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8017617

If there is agreement that getId() is fundamentally flawed and cannot be
depended upon, even for internal JDK implementation, then I see this as
pretty good justification to add a new final method to return the
stable/final Id.

I can float this on the OpenJDK mailing lists and see if there are any
objections.

-Chris.

>
> Nathan Reynolds
> <http://psr.us.oracle.com/wiki/index.php/User:Nathan_Reynolds> |
> Architect | 602.333.9091
> Oracle PSR Engineering <http://psr.us.oracle.com/> | Server Technology
> On 6/25/2013 11:30 AM, Chris Dennis wrote:
>> Yes, sadly people will insist on hanging themselves whenever we give
>> them enough rope.  I don't think I've actually seen someones code that
>> did this, but I have seen it's fallout:
>> http://forums.terracotta.org/forums/posts/list/0/2116.page
>>
>> Something like getStableId() would seem like a good idea, although I
>> strongly suspect this debate has occurred within Sun/Oracle and there
>> are probably good technical reasons why it's not happened yet.  What I
>> would like to see as a minimum is someone adding some stronger wording
>> to the Javadoc for the method.
>>
>> Chris
>>
>> On 6/25/13 2:02 PM, "Nathan Reynolds" <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>> Wow!  Final getId() would break user code?  Interesting.  Are there
>> any examples?  I can dream up a few but I am curious what is out in
>> the wild.
>>
>> I guess we need another method in Thread which is declared final and
>> returns the "tid" member variable value.  Maybe it could be called
>> getFinalId(), getImmutableId(), getFirmId(), or getStableId().
>>
>> Nathan Reynolds
>> <http://psr.us.oracle.com/wiki/index.php/User:Nathan_Reynolds> |
>> Architect | 602.333.9091
>> Oracle PSR Engineering <http://psr.us.oracle.com/> | Server Technology
>> On 6/25/2013 10:56 AM, Chris Dennis wrote:
>>> The issue about getId() not being final has been thrown about forever
>>> – it's not going to be fixed because it would cause too much breakage
>>> in user code.  Seems strange to me though given that we know the non
>>> finality of getId allows users to do this, we aren't avoiding relying
>>> on that method – why don't we have some other way for library code
>>> (JDK or third-party) to get access to a unique identifier for a thread?
>>>
>>> On 6/25/13 1:24 PM, "Nathan Reynolds" <[hidden email]
>>> <mailto:[hidden email]>> wrote:
>>>
>>> The JavaDoc for Thread.getId() says "...thread ID is unique..." so I
>>> don't think this is a bug in RRWL.  Maybe Thread.getId() should be
>>> declared final.
>>>
>>> We might want to consider going as far as declaring the member field
>>> "tid" as final.  This could be done via "private long tid =
>>> nextThreadID();"
>>>
>>> I find it very interesting that threadInitNumber and threadSeqNumber
>>> are both used in the Thread class.  It seems we only need 1.  It
>>> seems that the constructor should use "Thread-" + tid for a thread
>>> name.  In fact, the name could read "Thread-10" and the tid could be
>>> 7 because there is a race between when the name is generated and the
>>> tid is set.  The mismatch probably doesn't matter functionally.
>>> However, it could make it easier for debugging.
>>>
>>> Nathan Reynolds
>>> <http://psr.us.oracle.com/wiki/index.php/User:Nathan_Reynolds> |
>>> Architect | 602.333.9091
>>> Oracle PSR Engineering <http://psr.us.oracle.com/> | Server Technology
>>> On 6/25/2013 9:50 AM, Chris Dennis wrote:
>>>> Hi All,
>>>>
>>>> While dealing with a customer issue, I ran in to the possibility of
>>>> breaking a RRWL by feeding in Thread instances with colliding thread-ids.
>>>> Inside RRWL the cachedHoldCounter logic assumes that getId() will return a
>>>> unique identifier for a thread.  Do people consider this a bug in RRWL or
>>>> not? (I think it would be agreed that this is also a bug in the
>>>> subclassing of Thread)
>>>>
>>>> Regards,
>>>>
>>>> Chris Dennis
>>>>
>>>> public static void main(String[] args) {
>>>>    final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
>>>>    final CyclicBarrier barrier = new CyclicBarrier(2);
>>>>
>>>>    Thread t1 = new EvilThread() {
>>>>      public void run() {
>>>>        try {
>>>>          lock.readLock().lock();
>>>>          barrier.await();
>>>>          //T2 locks
>>>>          barrier.await();
>>>>          //T3 locks
>>>>          //T3 unlocks
>>>>          barrier.await();
>>>>          //T2 unlocks
>>>>          barrier.await();
>>>>          lock.readLock().unlock();
>>>>        } catch (Exception e) {
>>>>          e.printStackTrace();
>>>>        }
>>>>      }
>>>>
>>>>    };
>>>>    Thread t2 = new EvilThread() {
>>>>      public void run() {
>>>>        try {
>>>>          //T1 locks
>>>>                    barrier.await();
>>>>                    lock.readLock().lock();
>>>>                    barrier.await();
>>>>                    //T3 locks
>>>>                    //T3 unlocks
>>>>                    barrier.await();
>>>>                    lock.readLock().unlock();
>>>>                    barrier.await();
>>>>                    //T1 unlocks
>>>>        } catch (Exception e) {
>>>>          e.printStackTrace();
>>>>        }
>>>>      }
>>>>
>>>>    };
>>>>    Thread t3 = new EvilThread() {
>>>>      public void run() {
>>>>        try {
>>>>          //T1 locks
>>>>          barrier.await();
>>>>                    //T2 locks
>>>>                    barrier.await();
>>>>                    lock.readLock().lock();
>>>>                    lock.readLock().unlock();
>>>>                    barrier.await();
>>>>                    //T2 unlocks
>>>>                    barrier.await();
>>>>                    //T3 unlocks
>>>>        } catch (Exception e) {
>>>>          e.printStackTrace();
>>>>        }
>>>>      }
>>>>
>>>>    };
>>>>
>>>>    t1.start();
>>>>    t2.start();
>>>>    t3.start();
>>>>    }
>>>>
>>>>
>>>>    static class EvilThread extends Thread {
>>>>    public long getId() {
>>>>      return 42L;
>>>>    }
>>>>    }
>>>>
>>>>
>>>> _______________________________________________
>>>> Concurrency-interest mailing list
>>>> [hidden email]://cs.oswego.edu/mailman/listinfo/concurrency-interest
>>>
>>> _______________________________________________ Concurrency-interest
>>> mailing list [hidden email]
>>> <mailto:[hidden email]>
>>> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>>
>
>
>
> _______________________________________________
> Concurrency-interest mailing list
> [hidden email]
> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: RRWL with 'bad' Thread.getId() implementations

Aleksey Shipilev-2
In reply to this post by Nathan Reynolds-2
On 06/25/2013 09:24 PM, Nathan Reynolds wrote:
> The JavaDoc for Thread.getId() says "...thread ID is unique..." so I
> don't think this is a bug in RRWL.  

+1. Intentionally breaking the Thread contract does seem bad.

> I find it very interesting that threadInitNumber and threadSeqNumber are
> both used in the Thread class.  It seems we only need 1.  It seems that
> the constructor should use "Thread-" + tid for a thread name.  In fact,
> the name could read "Thread-10" and the tid could be 7 because there is
> a race between when the name is generated and the tid is set.  The
> mismatch probably doesn't matter functionally.  However, it could make
> it easier for debugging.

In addition, we could finally eliminate those synchronized{} in JDK 8.
(Since Thread is the primordial class, you can't easily use atomics
there, but now as we have Unsafe.getAndAdd*... ;)

This looks like a good and simple task for a newcomer to OpenJDK. If
anyone from the community is interested in prototyping the patch,
testing it, and pushing through OpenJDK, but does not know where to
start, PM me ;)

-Aleksey.
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: RRWL with 'bad' Thread.getId() implementations

Chris Dennis
In a similar vein, the following code doesn't behave like you'd expect
either (although it's arguable as to whether this is a reasonable way for
RRWL to behave in this circumstance).  Note here that this is totally
legal per the getId() contract, because the id doesn't get recycled until
after the join returns.

Chris

  public static void main(String[] args) throws InterruptedException {
    final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
    final CyclicBarrier barrier = new CyclicBarrier(2);
   
    Thread t1 = new LegalThread(1) {

      @Override
      public void run() {
        try {
          lock.readLock().lock();
          barrier.await();
          //T2a locks
          //T2b unlocks
          barrier.await();
          lock.readLock().unlock();
        } catch (Exception e) {
          e.printStackTrace();
        }
      }
    };
    Thread t2a = new LegalThread(2) {

      @Override
      public void run() {
        try {
          //T1 locks
          barrier.await();
          lock.readLock().lock();
        } catch (Exception e) {
          e.printStackTrace();
        }
      }
     
    };
   
    t1.start();
    t2a.start();
    t2a.join();
   
    Thread t2b = new LegalThread(2) {

      @Override
      public void run() {
        try {
          lock.readLock().unlock();
          barrier.await();
        } catch (Exception e) {
          e.printStackTrace();
        }
      }
     
    };
    t2b.start();
  }
 
  static class LegalThread extends Thread {

    private final int id;
   
    public LegalThread(int id) {
      this.id = id;
    }
   
    @Override
    public long getId() {
      return id;
    }
  }



On 6/25/13 3:24 PM, "Aleksey Shipilev" <[hidden email]> wrote:

>On 06/25/2013 09:24 PM, Nathan Reynolds wrote:
>> The JavaDoc for Thread.getId() says "...thread ID is unique..." so I
>> don't think this is a bug in RRWL.
>
>+1. Intentionally breaking the Thread contract does seem bad.
>
>> I find it very interesting that threadInitNumber and threadSeqNumber are
>> both used in the Thread class.  It seems we only need 1.  It seems that
>> the constructor should use "Thread-" + tid for a thread name.  In fact,
>> the name could read "Thread-10" and the tid could be 7 because there is
>> a race between when the name is generated and the tid is set.  The
>> mismatch probably doesn't matter functionally.  However, it could make
>> it easier for debugging.
>
>In addition, we could finally eliminate those synchronized{} in JDK 8.
>(Since Thread is the primordial class, you can't easily use atomics
>there, but now as we have Unsafe.getAndAdd*... ;)
>
>This looks like a good and simple task for a newcomer to OpenJDK. If
>anyone from the community is interested in prototyping the patch,
>testing it, and pushing through OpenJDK, but does not know where to
>start, PM me ;)
>
>-Aleksey.
>_______________________________________________
>Concurrency-interest mailing list
>[hidden email]
>http://cs.oswego.edu/mailman/listinfo/concurrency-interest


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: RRWL with 'bad' Thread.getId() implementations

Aleksey Shipilev-2
On 06/25/2013 11:54 PM, Chris Dennis wrote:
> In a similar vein, the following code doesn't behave like you'd expect
> either (although it's arguable as to whether this is a reasonable way for
> RRWL to behave in this circumstance).  Note here that this is totally
> legal per the getId() contract, because the id doesn't get recycled until
> after the join returns.

Now that's a good example!

This highlights the potential problem in RRWL. The code should instead
throw the IllegalMonitorStateException in t2b.run(). I wonder what harm
is done if we disregard the comment there and store the reference to
Thread instead.

        /**
         * A counter for per-thread read hold counts.
         * Maintained as a ThreadLocal; cached in cachedHoldCounter
         */
        static final class HoldCounter {
            int count = 0;
            // Use id, not reference, to avoid garbage retention
            final long tid = Thread.currentThread().getId();
        }

        static final class ThreadLocalHoldCounter
            extends ThreadLocal<HoldCounter> {
            public HoldCounter initialValue() {
                return new HoldCounter();
            }
        }

I don't see the problem off-hand: if thread dies, so do the associated
ThreadLocalMap-s. RRWL does not reference the live
ThreadLocalHoldCounter when nobody is holding the read lock... What else
is missing?

-Aleksey.
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: RRWL with 'bad' Thread.getId() implementations

Chris Dennis
(Sorry for mixing top and bottom posting - hope nobody is offended!)

You'll need to blank the cachedHoldCounter when a thread releases it's
last hold as well, right?  Otherwise we still leak a reference to the last
reader to take this lock, even if that thread has unlocked fully.

On 6/25/13 4:09 PM, "Aleksey Shipilev" <[hidden email]> wrote:

>On 06/25/2013 11:54 PM, Chris Dennis wrote:
>> In a similar vein, the following code doesn't behave like you'd expect
>> either (although it's arguable as to whether this is a reasonable way
>>for
>> RRWL to behave in this circumstance).  Note here that this is totally
>> legal per the getId() contract, because the id doesn't get recycled
>>until
>> after the join returns.
>
>Now that's a good example!
>
>This highlights the potential problem in RRWL. The code should instead
>throw the IllegalMonitorStateException in t2b.run(). I wonder what harm
>is done if we disregard the comment there and store the reference to
>Thread instead.
>
>        /**
>         * A counter for per-thread read hold counts.
>         * Maintained as a ThreadLocal; cached in cachedHoldCounter
>         */
>        static final class HoldCounter {
>            int count = 0;
>            // Use id, not reference, to avoid garbage retention
>            final long tid = Thread.currentThread().getId();
>        }
>
>        static final class ThreadLocalHoldCounter
>            extends ThreadLocal<HoldCounter> {
>            public HoldCounter initialValue() {
>                return new HoldCounter();
>            }
>        }
>
>I don't see the problem off-hand: if thread dies, so do the associated
>ThreadLocalMap-s. RRWL does not reference the live
>ThreadLocalHoldCounter when nobody is holding the read lock... What else
>is missing?
>
>-Aleksey.


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: RRWL with 'bad' Thread.getId() implementations

Aleksey Shipilev-2
Right. In fact, it is specifically said:

        /**
         * [snip]
         *
         * <p>Can outlive the Thread for which it is caching the read
         * hold count, but avoids garbage retention by not retaining a
         * reference to the Thread.
         *
         * [snip]
         */
        private transient HoldCounter cachedHoldCounter;

Other ideas how to solve this?

-Aleksey.

On 06/26/2013 12:27 AM, Chris Dennis wrote:

> (Sorry for mixing top and bottom posting - hope nobody is offended!)
>
> You'll need to blank the cachedHoldCounter when a thread releases it's
> last hold as well, right?  Otherwise we still leak a reference to the last
> reader to take this lock, even if that thread has unlocked fully.
>
> On 6/25/13 4:09 PM, "Aleksey Shipilev" <[hidden email]> wrote:
>
>> On 06/25/2013 11:54 PM, Chris Dennis wrote:
>>> In a similar vein, the following code doesn't behave like you'd expect
>>> either (although it's arguable as to whether this is a reasonable way
>>> for
>>> RRWL to behave in this circumstance).  Note here that this is totally
>>> legal per the getId() contract, because the id doesn't get recycled
>>> until
>>> after the join returns.
>>
>> Now that's a good example!
>>
>> This highlights the potential problem in RRWL. The code should instead
>> throw the IllegalMonitorStateException in t2b.run(). I wonder what harm
>> is done if we disregard the comment there and store the reference to
>> Thread instead.
>>
>>        /**
>>         * A counter for per-thread read hold counts.
>>         * Maintained as a ThreadLocal; cached in cachedHoldCounter
>>         */
>>        static final class HoldCounter {
>>            int count = 0;
>>            // Use id, not reference, to avoid garbage retention
>>            final long tid = Thread.currentThread().getId();
>>        }
>>
>>        static final class ThreadLocalHoldCounter
>>            extends ThreadLocal<HoldCounter> {
>>            public HoldCounter initialValue() {
>>                return new HoldCounter();
>>            }
>>        }
>>
>> I don't see the problem off-hand: if thread dies, so do the associated
>> ThreadLocalMap-s. RRWL does not reference the live
>> ThreadLocalHoldCounter when nobody is holding the read lock... What else
>> is missing?
>>
>> -Aleksey.
>
>

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: RRWL with 'bad' Thread.getId() implementations

bestsss
WeakReference obviously,
Like I've said: Thread.id is a cheap hack for == and weak reference. Alternative incl. eetop field.

Stanimir

On Tue, Jun 25, 2013 at 11:37 PM, Aleksey Shipilev <[hidden email]> wrote:
Right. In fact, it is specifically said:

        /**
         * [snip]
         *
         * <p>Can outlive the Thread for which it is caching the read
         * hold count, but avoids garbage retention by not retaining a
         * reference to the Thread.
         *
         * [snip]
         */
        private transient HoldCounter cachedHoldCounter;

Other ideas how to solve this?

-Aleksey.

On 06/26/2013 12:27 AM, Chris Dennis wrote:
> (Sorry for mixing top and bottom posting - hope nobody is offended!)
>
> You'll need to blank the cachedHoldCounter when a thread releases it's
> last hold as well, right?  Otherwise we still leak a reference to the last
> reader to take this lock, even if that thread has unlocked fully.
>
> On 6/25/13 4:09 PM, "Aleksey Shipilev" <[hidden email]> wrote:
>
>> On 06/25/2013 11:54 PM, Chris Dennis wrote:
>>> In a similar vein, the following code doesn't behave like you'd expect
>>> either (although it's arguable as to whether this is a reasonable way
>>> for
>>> RRWL to behave in this circumstance).  Note here that this is totally
>>> legal per the getId() contract, because the id doesn't get recycled
>>> until
>>> after the join returns.
>>
>> Now that's a good example!
>>
>> This highlights the potential problem in RRWL. The code should instead
>> throw the IllegalMonitorStateException in t2b.run(). I wonder what harm
>> is done if we disregard the comment there and store the reference to
>> Thread instead.
>>
>>        /**
>>         * A counter for per-thread read hold counts.
>>         * Maintained as a ThreadLocal; cached in cachedHoldCounter
>>         */
>>        static final class HoldCounter {
>>            int count = 0;
>>            // Use id, not reference, to avoid garbage retention
>>            final long tid = Thread.currentThread().getId();
>>        }
>>
>>        static final class ThreadLocalHoldCounter
>>            extends ThreadLocal<HoldCounter> {
>>            public HoldCounter initialValue() {
>>                return new HoldCounter();
>>            }
>>        }
>>
>> I don't see the problem off-hand: if thread dies, so do the associated
>> ThreadLocalMap-s. RRWL does not reference the live
>> ThreadLocalHoldCounter when nobody is holding the read lock... What else
>> is missing?
>>
>> -Aleksey.
>
>

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: RRWL with 'bad' Thread.getId() implementations

Martin Buchholz-3



On Tue, Jun 25, 2013 at 1:55 PM, Stanimir Simeonoff <[hidden email]> wrote:
WeakReference obviously,
Like I've said: Thread.id is a cheap hack for == and weak reference. Alternative incl. eetop field.


We the maintainers of RRWL dislike the use of both ThreadLocals and WeakReferences and wish that RRWL could be implemented without either. 

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: RRWL with 'bad' Thread.getId() implementations

Aleksey Shipilev-2
On 06/26/2013 01:13 AM, Martin Buchholz wrote:

> On Tue, Jun 25, 2013 at 1:55 PM, Stanimir Simeonoff
> <[hidden email] <mailto:[hidden email]>> wrote:
>
>     WeakReference obviously,
>     Like I've said: Thread.id is a cheap hack for == and weak reference.
>     Alternative incl. eetop field.
>
>
> We the maintainers of RRWL dislike the use of both ThreadLocals and
> WeakReferences and wish that RRWL could be implemented without either.

That's why you have "invented" StampedLock, all right!

On the serious note, we need to frame this discussion a bit. Do you want
the bug against RRWL with the testcase Chris had came up with? (That is,
do you agree this is the bug in RRWL?)

-Aleksey.
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: RRWL with 'bad' Thread.getId() implementations

Martin Buchholz-3



On Tue, Jun 25, 2013 at 2:16 PM, Aleksey Shipilev <[hidden email]> wrote:
On 06/26/2013 01:13 AM, Martin Buchholz wrote:
On the serious note, we need to frame this discussion a bit. Do you want
the bug against RRWL with the testcase Chris had came up with? (That is,
do you agree this is the bug in RRWL?)

I think I agree this is a small bug in RRWL, but I suspect any attempt to cure it would be worse than the disease.

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: RRWL with 'bad' Thread.getId() implementations

Aleksey Shipilev-2
On 06/26/2013 01:20 AM, Martin Buchholz wrote:

>
>
>
> On Tue, Jun 25, 2013 at 2:16 PM, Aleksey Shipilev
> <[hidden email] <mailto:[hidden email]>> wrote:
>
>     On 06/26/2013 01:13 AM, Martin Buchholz wrote:
>     On the serious note, we need to frame this discussion a bit. Do you want
>     the bug against RRWL with the testcase Chris had came up with? (That is,
>     do you agree this is the bug in RRWL?)
>
>
> I think I agree this is a small bug in RRWL, but I suspect any attempt
> to cure it would be worse than the disease.

I think the same. Nevertheless, the issue is recorded as CR 8017739.
Will be publicly available soon as:
 http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8017739

Thanks everyone!

-Aleksey.

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
12