Using ReentrantLock

R

RVic

if (null != response) {
I haev an output stream passed to a thread wherein I wish to write to
the outputstream, but have anyone else locked out from writing to it
while the particular handed-off-to thread writes to it.

I am a little unsure about using ReentrantLock to do this. In my code
below, which compiles (so what!) can anyone tell if I am using it
correctly or incorrectly or what I might have that's incorrect or
missing? Thanks, Rvince

ReentrantLock lockObject = new ReentrantLock(false);
try {
lockObject.lock();
output.write(merchLinklengthBytes(response));
output.write(headerControl);
output.write(response.getBytes());
}
catch (Exception e) {
e.printStackTrace();
}
finally {
if (null != lockObject) {
lockObject.unlock();
}
}
}
 
M

markspace

RVic said:
if (null != response) {
I haev an output stream passed to a thread wherein I wish to write to
the outputstream, but have anyone else locked out from writing to it
while the particular handed-off-to thread writes to it.

I am a little unsure about using ReentrantLock to do this. In my code
below, which compiles (so what!) can anyone tell if I am using it
correctly or incorrectly or what I might have that's incorrect or
missing? Thanks, Rvince

ReentrantLock lockObject = new ReentrantLock(false);


I don't think this can work because you allocate a new lock each time.
Each invocation will get a new lock, which is unlocked, and then just
lock it. Hence there's no co-ordination between the various different
users.

Use an instance variable instead. Make one lock, and make sure everyone
(all callers) use the same lock object.


public class MyOutputStream implements OutputStream
{
ReentrantLock lockObject = new ReentrantLock(false);

public void writeStuff() {
try {
lockObject.lock();
output.write(merchLinklengthBytes(response));
output.write(headerControl);
output.write(response.getBytes());
}
catch (Exception e) {
e.printStackTrace();
}
finally {
if (null != lockObject) {
lockObject.unlock();
}
}
}
} // writeStuff
} // class MyOutputStream
 
L

Lew

Make one lock, and make sure everyone
(all callers) use the same lock object.

public class MyOutputStream implements OutputStream
{
final // also, can be private and probably should be
   ReentrantLock lockObject = new ReentrantLock(false);

'lockObject' is not a great variable name.
   public void writeStuff() {

Under what circumstances can 'lockObject' equal 'null' here?

How do those circumstances differ from what pertains at the
'lockObject.lock()' call?
 
M

markspace

Lew said:
Under what circumstances can 'lockObject' equal 'null' here?

How do those circumstances differ from what pertains at the
'lockObject.lock()' call?


Yeah, good point. It's a pretty cheesy example. I just seized on the
first thing I noticed in the OP's problem and posted that. I didn't
even look at the docs for ReetrantLock. So there's any manner of things
that could be off here.

OP: caveat emptor. No one is doing you homework/job related work for
you. Best to check everything out yourself before accepting code from
the 'net.
 
K

Knute Johnson

markspace said:
I don't think this can work because you allocate a new lock each time.
Each invocation will get a new lock, which is unlocked, and then just
lock it. Hence there's no co-ordination between the various different
users.

Use an instance variable instead. Make one lock, and make sure everyone
(all callers) use the same lock object.

and make it final too.
public class MyOutputStream implements OutputStream
{
ReentrantLock lockObject = new ReentrantLock(false);

public void writeStuff() {
} // writeStuff
} // class MyOutputStream
 
R

Roedy Green

ReentrantLock lockObject = new ReentrantLock(false);
try {
lockObject.lock();
output.write(merchLinklengthBytes(response));
output.write(headerControl);
output.write(response.getBytes());
}

There is no lock protection here. The lock will always succeed since
no other thread even sees the locally referenced lockObject.

Any kind of lockObject has to be an instance or static variable, or
else no other thread will see it.
 
A

Arne Vajhøj

Roedy said:
There is no lock protection here. The lock will always succeed since
no other thread even sees the locally referenced lockObject.

Yes. This has already been clearly stated by others.
Any kind of lockObject has to be an instance or static variable, or
else no other thread will see it.

Not correct. Local variables can also be passed on to threads.

Arne
 
A

Arved Sandstrom

Arne said:
Yes. This has already been clearly stated by others.


Not correct. Local variables can also be passed on to threads.

Arne

I'm not sure what you mean by the latter statement. If you mean that if
a local variable is initialized by a shared object that all threads
accessing the local reference will be operating on the same thing, then
I understand what you said.

AHS
 
R

RVince

Thanks guys,
Please don't get the impression I am looking for someone to do my
homework -- rather, I'm very unsure of my understanding of this from
the docs, and, like everyone else here, I need to write solid code; I
am hoping tod exactly what I have received here, a solid review and
commentary from my peers.

It seems based on everyone's comments (please correct me if I am
wrong), that all I really need do is make lockObject a static variable
from the calling thread. This way, all see it, and there is only one
copy that all threads called would be working from. This seems simple,
solid and robust. For example:

class MyCallingClass{
static ReentrantLock lockObject = new ReentrantLock(false);
myCalledThread=newMyCalledThread(lockObject);
myCalledThread.t.start();

class MyCalledThread{
Thread t;
ReentrantLock lockObject;
MyCalledThread(ReentrantLock lockObject){
this.lockObject=lockObject;
}
public void run(){
try {
lockObject.lock();
output.write(merchLinklengthBytes(response));
output.write(headerControl);
output.write(response.getBytes());
}
catch (Exception e) {
e.printStackTrace();
}
finally {
if (null != lockObject) {
lockObject.unlock();
}
}
}
}
}

Does anyone see any glaring problem - grotesquely stupid mistake I am
making here. Again, I'm just very unsure on this approach to doing
this, and trying to keep it as simple & robust as possible. Based on
your critiques, I think this accomplishes that end. But in X decades
of writing code, I can;t think of ANYTIME, ANYTHING, has ever run as I
thought it would on the first pass! Thanks, RVince
 
M

markspace

RVince said:
It seems based on everyone's comments (please correct me if I am
wrong), that all I really need do is make lockObject a static variable

No, you don't need to make the lock static. No, you don't need to do
anything from the calling thread.
from the calling thread. This way, all see it, and there is only one
copy that all threads called would be working from. This seems simple,

Even if your class is a singleton, you should still use an instance
variable, not a static variable.

solid and robust. For example:

I think your code may have got worse.

class MyCallingClass{
static ReentrantLock lockObject = new ReentrantLock(false);
myCalledThread=newMyCalledThread(lockObject);
myCalledThread.t.start();


Does this code even run? Can you compile this first and show the output
in your post, please? I think a lack of a concrete example is really
starting to hinder the discussion.

What you did here is totally different than your original post. Your
first post was a synchronized or thread-safe object. That's standard
enough. Now you have a global, static lock that anyone can acquire, and
everyone making a particular set of calls has to remember to take the
lock. This seems several large steps in the wrong direction.

If nothing else, please go with encapsulating the lock into an object
that does the work for you. That's basic, basic software engineering.
 
K

Knute Johnson

RVince said:
Thanks guys,
Please don't get the impression I am looking for someone to do my
homework -- rather, I'm very unsure of my understanding of this from
the docs, and, like everyone else here, I need to write solid code; I
am hoping tod exactly what I have received here, a solid review and
commentary from my peers.

It seems based on everyone's comments (please correct me if I am
wrong), that all I really need do is make lockObject a static variable
from the calling thread. This way, all see it, and there is only one
copy that all threads called would be working from. This seems simple,
solid and robust. For example:

class MyCallingClass{
static ReentrantLock lockObject = new ReentrantLock(false);
myCalledThread=newMyCalledThread(lockObject);
myCalledThread.t.start();

class MyCalledThread{
Thread t;
ReentrantLock lockObject;
MyCalledThread(ReentrantLock lockObject){
this.lockObject=lockObject;
}
public void run(){
try {
lockObject.lock();
output.write(merchLinklengthBytes(response));
output.write(headerControl);
output.write(response.getBytes());
}
catch (Exception e) {
e.printStackTrace();
}
finally {
if (null != lockObject) {
lockObject.unlock();
}
}
}
}
}

Does anyone see any glaring problem - grotesquely stupid mistake I am
making here. Again, I'm just very unsure on this approach to doing
this, and trying to keep it as simple & robust as possible. Based on
your critiques, I think this accomplishes that end. But in X decades
of writing code, I can;t think of ANYTIME, ANYTHING, has ever run as I
thought it would on the first pass! Thanks, RVince

I doesn't need to be static. It should final. You really don't need a
reentrant lock for this job. Just us synchronized.
 
L

Lew

RVince said:
It seems based on everyone's comments (please correct me if I am
wrong), that all I really need do is make lockObject a static variable

As others have said, no, do not make it static.
from the calling thread. This way, all see it, and there is only one
copy that all threads called would be working from. This seems simple,
solid and robust. For example:

class MyCallingClass{
static ReentrantLock lockObject = new ReentrantLock(false);
myCalledThread=newMyCalledThread(lockObject);
^^
This will not compile.
myCalledThread.t.start();
^^
This will not compile. Even if it did, since thread 't' never is assigned
anything you'd get a NullPointerException.
class MyCalledThread{

Why do you use a nested class? Why is it an inner class?
Thread t;
ReentrantLock lockObject;
MyCalledThread(ReentrantLock lockObject){
this.lockObject=lockObject;

Why pass 'lockObject' to a constructor when the outer class variable is
already visible to the inner class? Since you do pass it in, why is the
inner-class variable not 'final'?
}
public void run(){
try {
lockObject.lock();
output.write(merchLinklengthBytes(response));
output.write(headerControl);
output.write(response.getBytes());
}
catch (Exception e) {
e.printStackTrace();
}
finally {
if (null != lockObject) {

I asked before, and you didn't answer: Under what circumstances could
'lockObject == null'? Why don't those circumstances pertain to the call to
'lockObject.lock()'?

Perhaps you will answer this time. They're important questions, and asked in
order to help you get certain things clear. You should not ignore them. They
will help you.
lockObject.unlock();
}
}
}
}
}

Does anyone see any glaring problem - grotesquely stupid mistake I am
making here. Again, I'm just very unsure on this approach to doing
this, and trying to keep it as simple & robust as possible. Based on
your critiques, I think this accomplishes that end. But in X decades
of writing code, I can;t think of ANYTIME, ANYTHING, has ever run as I
thought it would on the first pass!

That could be an indicator that you need to revise your mental model of how
these programs behave.
 
A

Arne Vajhøj

Arved said:
I'm not sure what you mean by the latter statement. If you mean that if
a local variable is initialized by a shared object that all threads
accessing the local reference will be operating on the same thing, then
I understand what you said.

I was actually talking about the similar but slightly different
situation where the object are made available after assignment.

Object o = new Object();
something(o);
// now o could be shared among threads if the something made it so

Arne
 
A

Arved Sandstrom

markspace said:
No, you don't need to make the lock static. No, you don't need to do
anything from the calling thread.


Even if your class is a singleton, you should still use an instance
variable, not a static variable.



I think your code may have got worse.




Does this code even run? Can you compile this first and show the output
in your post, please? I think a lack of a concrete example is really
starting to hinder the discussion.

What you did here is totally different than your original post. Your
first post was a synchronized or thread-safe object. That's standard
enough. Now you have a global, static lock that anyone can acquire, and
everyone making a particular set of calls has to remember to take the
lock. This seems several large steps in the wrong direction.

If nothing else, please go with encapsulating the lock into an object
that does the work for you. That's basic, basic software engineering.
I'm with Mark here, especially concerning his last paragraph. Maybe you
should step back a bit, take a few deep breaths, and recapture a high
level view of what it is you're trying to do here. By the way, reading
the Java tutorial on concurrency would be a good idea, and having a copy
of "Java Concurrency In Practice" is even better...but that's an aside.

Concerning Mark's last point, think of the use of ReentrantLock (or Lock
in general) as _basically_ replacing a use of "synchronized". (Similar
replacement semantics occur between java.util.concurrent.locks.Condition
and the Object monitor methods like wait() and notifyAll()).

Running (pun not intended) with that idea, why would you use
"synchronized" (or ReentrantLock) on a block or method? Because you
don't want 2 or more threads executing the code in that block or method
at the same time. These are your callers - don't worry so much about
them right now. Like Mark said, concern yourself at the moment with the
callee class, which contains the method that many threads could invoke.
_That_ is the class that will contain the ReentrantLock (a final
instance variable [*]). Each "synchronized" (no use of that keyword any
more) method will use the idiom you see at the top of
http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/locks/ReentrantLock.html.

This should get you back on the right track. You need to logically
understand what it is that's really happening before you start coding in
the synchronization itself. In this particular case, unless I'm missing
something, the concurrency situation is as basic as it gets.

AHS

* This is not a hard and fast rule, but again, think about what it is
that you're doing. You have an output stream that is associated with an
instance of the callee class (my assumption). You want only one thread
at a time to be able to write to this stream. You basically want the
ReentrantLock to exist at the same scope and the same place as the
output stream, hence it's an instance variable. "final" because no
thread should change it.
 
R

RVince

No, I havent compiled it because i have it at my day job -- clearly I
am misunderstanding what you guys are telling me. There's a level of
abstraction here that I am just not seeing, and I don;t know why --
why I cannot see what must be so simple to accomplish, that being that
only one thread at a time should be allowed to write to the
outputstream. I just don't see what you guys are telling me now. -RVic
 
L

Lew

Arne said:
I was actually talking about the similar but slightly different
situation where the object are made available after assignment.

Object o = new Object();
something(o);
// now o could be shared among threads if the something made it so

Provided 'o' is 'final'.
 
A

Arved Sandstrom

RVince said:
No, I havent compiled it because i have it at my day job -- clearly I
am misunderstanding what you guys are telling me. There's a level of
abstraction here that I am just not seeing, and I don;t know why --
why I cannot see what must be so simple to accomplish, that being that
only one thread at a time should be allowed to write to the
outputstream. I just don't see what you guys are telling me now. -RVic

As I expressed in another post, step back, take a deep breath, and look
at what's really happening here. The central issue is that you have a
block of code - the statements that access the output stream - that can
only be used by one thread at a time. Let's say that this block of code
is inside a method in some class (a logger class, say). The various
threads all eventually execute some other code that invokes this method.

Without using ReentrantLock the simplest way of ensuring
one-thread-at-a-time access is to synchronize that method. That's it.

If you wish to use ReentrantLock instead of the "synchronized" keyword,
in this scenario, the replacement technique boils basically down to:

1) initialize an instance variable to a new ReentrantLock - all the
threads using the method will see the same lock. It's final to make sure
that nothing changes it. It's not static because I'm assuming that the
class is instantiated and that's how the "synchronized" method is
called. You could put forward a good case for having this method be
static, in which case so would the lock be.

2) wrap your code that is to be synchronized inside a try block;

3) call lock() immediately before the try block;

4) call unlock() inside the finally block.

If you are passing in the output stream from outside just make sure that
it doesn't get used somewhere else. If it does _not_ get used
anywhere else it's probably easiest to have the output stream reside
inside the class containing the called method, as an instance variable.

In this particular scenario I'd recommend trying to make this work with
the good ol' "synchronized" keyword, and understanding that completely.
You're not gaining anything by using ReentrantLock here. Swap in
ReentrantLock only when you understand the use of "synchronized". At
that point you'll also be nicely positioned to exploit the real
advantages of ReentrantLocks, including creating Conditions.

AHS
 
L

Lew

Arne said:
I can not see why final or non-final should make a difference
here.

First there is the JLS that says (17.4.1):
Local variables (§14.4), formal method parameters (§8.4.1) or
exception handler parameters are never shared between threads
and are unaffected by the memory model.

The only way I know of to get a thread to share a local variable value is as
an inner class that refers to a variable in the invoking context, which
requires that the outer variable be 'final'.

JLS 8.1.3:
 
A

Arne Vajhøj

Lew said:
First there is the JLS that says (17.4.1):

The only way I know of to get a thread to share a local variable value
is as an inner class that refers to a variable in the invoking context,
which requires that the outer variable be 'final'.

JLS 8.1.3:

All very true.

But bot Object in my example and ReentrantLock in the original post
are classes.

All the above you quote are valid for the reference itself, while
the discussion applies to the object pointed to by the reference.

Arne
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

No members online now.

Forum statistics

Threads
473,744
Messages
2,569,484
Members
44,903
Latest member
orderPeak8CBDGummies

Latest Threads

Top