Synchronized, wait, and notifyall.

[Danny]

Java Gurus:

The method below is called in a meeting application. It lives in a
Tomcat server thread, where it uses a
blocked thread to wait for up to 30 seconds to return an event. When
another thread posts an event for an attendee, it executes a notifyall
on the attendee.

Are there any holes? For example, what can I do to avoid changes to
the attendee, which was being waited upon, between the time the wait
was broken by the notifyall and the attendee is synchronized to
extract and reset its event?

Thanks very much for any advice,

Danny
=========================================================

public ArrayList<MeetingEvent> getMeetingEvents(String[]
session,
Meeting meeting){
ArrayList<MeetingEvent> events = null;
Attendee attendee = null;
synchronized(attendees){
attendee = attendees.get(session
[ServerUtil.MEMBERSHIP_ID] + "." +
meeting.meetingID + "." + session[ServerUtil.USER_ID]);
}
if( attendee != null ){
if( attendee.events.size() == 0 ){ //If
attendee has no events,
wait for 30 seconds.
try{
synchronized(attendee){
attendee.wait(30000);
}
}
catch (InterruptedException ignored){}
}
synchronized(attendee)
{ //If attendee has events save for the
return and empty for next event.
events = attendee.events;
attendee.events = new
ArrayList<MeetingEvent>();
}
}
return events;
} //End getMeetingEvents

Where MeetingEvent is:

public class MeetingEvent implements IsSerializable{
public int action;
public String[][] value;
public String membershipID;
public String meetingID;
public MeetingEvent(String membershipID, String meetingID, int
action, String[][] initialValue){
this.membershipID = membershipID;
this.meetingID = meetingID;
this.action = action;
value = initialValue;
}
public MeetingEvent(){}
}

 

[Lew]

The method below is called in a meeting application.  It lives in a
Tomcat server thread, where it uses a

It can be dangerous to explicitly spawn threads from a web app. You
do have several threading issues here, but I won't pretend to give a
complete analysis.

Part of the problem is that your example is incomplete. Read Andrew
Thompson's excellent essay on SSCCEs, mirrored at
<http://sscce.org/>

What version of Java are you using?

blocked thread to wait for up to 30 seconds to return an event.  When

The first is that your 'wait()' lock occurs inside another lock, which
could quite adversely affect performance.

another thread posts an event for an attendee, it executes a notifyall
on the attendee.

If the other thread needs to acquire the lock on 'attendees', it will
be hungry trying to do so, until the waiting thread releases its lock.

Are there any holes?  For example, what can I do to avoid changes to
the attendee, which was being waited upon,  between the time the wait
was broken by the notifyall and the attendee is synchronized to
extract and reset its event?

The 'attendee' is not fully guarded.

=========================================================

       public ArrayList<MeetingEvent> getMeetingEvents(String[]
session,
Meeting meeting){

You should return a 'List' instead of an 'ArrayList', unless there is
some particular reason to lock the implementation down like that.
Same with the declaration of 'events'.

               ArrayList<MeetingEvent> events = null;
               Attendee attendee = null;
               synchronized(attendees){
                       attendee = attendees.get(session
[ServerUtil.MEMBERSHIP_ID] + "." +
meeting.meetingID + "." + session[ServerUtil.USER_ID]);
               }

You might be better off releasing the lock on 'attendees' now, instead
of holding through all the lengthy stuff that might follow.

               if( attendee != null ){
                       if( attendee.events.size() == 0 ){   //If
attendee has no events,
wait for 30 seconds.

You are not synchronized on 'attendee' here, so it is possible for
this thread to see 'events' equal to 'null', or of size 0, even after
another thread has changed it.

                               try{
                                       synchronized(attendee){
                                               attendee.wait(30000);

Your indentation is far too aggressive for Usenet. You should limit
indent levels to four spaces or less each.

                                       }
                               }
                               catch (InterruptedException ignored){}
                       }
                       synchronized(attendee)

This confuses me.

{                         //If attendee has events save for the
return and empty for next event.
                               events = attendee.events;
                               attendee.events = new
ArrayList<MeetingEvent>();

Because the check for 'attendee.events.size() == 0' is not
synchronized, this change might not be visible to other threads.

                       }
               }
               return events;
       }       //End getMeetingEvents

That's all I have patience for.

 

[Tom Anderson]

The method below is called in a meeting application. It lives in a
Tomcat server thread, where it uses a blocked thread to wait for up to
30 seconds to return an event. When another thread posts an event for
an attendee, it executes a notifyall on the attendee.

Top suggestion: don't write this. Use a BlockingQueue from
java.util.concurrent instead. No holes, guaranteed (void where prohibited
by law). Attach a queue to each Attendee, have event-deliverers push
events onto the queue with add(), and the event-handler remove them with
the timeout version of poll().

You need to think about what happens when the queue is full - should
event-posters block until it isn't, drop the event on the floor, or deal
with the failure? There are three pushing methods, to reflect those
options.

I'll respond to your questions, but in the spirit of contemplation, rather
than of engineering.

Are there any holes?

Yes, you don't show us what the event-posting code looks like!

For example, what can I do to avoid changes to the attendee, which was
being waited upon, between the time the wait was broken by the notifyall
and the attendee is synchronized to extract and reset its event?

Why is that a problem? If "changes" means more events being posted, why
can't they just be added to the list?

If you really want it to not happen, you need to give the Attendee two
states: ready to be changed, and has been changed and ready to be read.
The simplest way to do this i can see is to use the state of the events
field - if it's null, it means events can be added, and if not, they
can't:

public class Attendee {
private static final long TIMEOUT = 30000 ;
private List<MeetingEvent> events ; // i don't *think* this needs to be transient
public synchronized List<MeetingEvent> getEvents() {
long deadline = System.currentTimeMillis() + TIMEOUT ;
try {
while (events == null) wait(deadline - System.currentTimeMillis()) ;
}
catch (InterruptedException e) {} // just fall through and probably return the empty list
if (events == null) return Collections.emptyList() ;
List eventsToReturn = events ;
events = null ;
notify() ;
return eventsToReturn ;
}
public synchronized void putEvents(List<MeetingEvent> events) throws InterruptedException {
long deadline = System.currentTimeMillis() + TIMEOUT ;
while (events != null) wait(deadline - System.currentTimeMillis()) ;
if (events != null) throw new InterruptedException("timed out waiting to post events") ;
this.events = events ;
notify() ;
}
}

This is pretty much the classic one-element channel, with some bells and
whistles.

Thanks very much for any advice,

Apart from the above, your code has pretty iffy OO design. Most of this
should be inside methods on Attendee, not in some external method.

And why doesn't Meeting have a map from membership and/or user ID to
Attendee? Then you wouldn't need a global map for all meetings.

public ArrayList<MeetingEvent> getMeetingEvents(String[]
session,
Meeting meeting){
ArrayList<MeetingEvent> events = null;
Attendee attendee = null;
synchronized(attendees){
attendee = attendees.get(session
[ServerUtil.MEMBERSHIP_ID] + "." +
meeting.meetingID + "." + session[ServerUtil.USER_ID]);
}
if( attendee != null ){
if( attendee.events.size() == 0 ){ //If
attendee has no events,
wait for 30 seconds.
try{
synchronized(attendee){
attendee.wait(30000);
}
}
catch (InterruptedException ignored){}
}
synchronized(attendee)
{ //If attendee has events save for the
return and empty for next event.
events = attendee.events;
attendee.events = new
ArrayList<MeetingEvent>();
}
}
return events;
} //End getMeetingEvents

Where MeetingEvent is:

.... probably the one class here whose implementation really doesn't
matter.

tom

 

[Mark Space]

try{
synchronized(attendee){
attendee.wait(30000);
}
}
catch (InterruptedException ignored){}

Ignoring interrupts like this is probably a bad idea. I'd restore the
interrupt in the catch block if you don't process it yourself.

In addition, stalling a Tomcat thread for 30 seconds seems like a bad
idea. I'd just refresh every 10-15 seconds in the browser (HTML or
JavaScript) and just return what "events" you have when you get the
request. I've used JavaScript chat apps that run on 10 second timers,
they feel very responsive.

 

[Lew]

[...]
               Attendee attendee = null;
               synchronized(attendees){
                       attendee = attendees.get(session
[ServerUtil.MEMBERSHIP_ID] + "." +
meeting.meetingID + "." + session[ServerUtil.USER_ID]);
               }

You might be better off releasing the lock on 'attendees' now, instead
of holding through all the lengthy stuff that might follow.

     Um, er, it looks like the bletcherous line-wrapping has
provoked a read error in your eye/brain interface ...

Oh. Oops. Teehee.

 

[Tom Anderson]

Ignoring interrupts like this is probably a bad idea. I'd restore the
interrupt in the catch block if you don't process it yourself.

It depends on what you think about interrupts.

What are interrupts for? What is the tao of interrupts?

I tend to think they're a way of saying to a thread "hey, i have something
to say to you, so if you're currently sat in a wait, a sleep, or a
blocking IO operation somewhere, stop that, and get back to running". That
means it isn't important to propagate an InterruptedException, but it is
important to stop doing whatever blocking thing was interrupted. Thus, i
think the OP's code is okay.

In addition, stalling a Tomcat thread for 30 seconds seems like a bad
idea. I'd just refresh every 10-15 seconds in the browser (HTML or
JavaScript) and just return what "events" you have when you get the
request.

Same here.

tom

 

[Mark Space]

It depends on what you think about interrupts.

What are interrupts for? What is the tao of interrupts?

In this case, I think it's more of a matter of "what does the container
think of interrupts?"

In other words, it's not my thread, and I don't know what Tomcat wants
to do on an interrupt. Since it's not my thread, I restore the
interrupt so that Tomcat can deal with it however it wants.

By swallowing the interrupt, you are preventing Tomcat from responding
to any internal mechanism that it might have implemented for handling
it's own threads.

 

[Arne Vajhøj]

Top suggestion: don't write this. Use a BlockingQueue from
java.util.concurrent instead. No holes, guaranteed (void where
prohibited by law). Attach a queue to each Attendee, have
event-deliverers push events onto the queue with add(), and the
event-handler remove them with the timeout version of poll().

Very good advice. With java.util.concurrent then user code
should very rarely need to use synchronized/wait/notifyAll.

Arne

 

[Arne Vajhøj]

In this case, I think it's more of a matter of "what does the container
think of interrupts?"

In other words, it's not my thread, and I don't know what Tomcat wants
to do on an interrupt. Since it's not my thread, I restore the
interrupt so that Tomcat can deal with it however it wants.

By swallowing the interrupt, you are preventing Tomcat from responding
to any internal mechanism that it might have implemented for handling
it's own threads.

I am pretty sure that Tomcat would be extremely "surprised"
if it received a checked exception like InterruptedException
from HttpServlet service that does not have such a throws.

Arne

 

[Arne Vajhøj]

Same here.

Me too.

I could be done with some refreshing JavaScript.

But performance and scalability would be much better if there
were some technology used client side that allowed for
push.

Arne

 

[Mark Space]

I am pretty sure that Tomcat would be extremely "surprised"
if it received a checked exception like InterruptedException
from HttpServlet service that does not have such a throws.

Probably, but I'm not talking about throwing InterruptedException. I'm
talking about restoring the interrupted status.

try {
synchronized(attendee) {
attendee.wait(30000);
}
}
catch (InterruptedException ignored){
Thread.currentThread().interrupt();
}

This is standard procedure for any InterruptedExcpetion that you catch
and don't respond to.

 

[Tom Anderson]

Me too.

I could be done with some refreshing JavaScript.

But performance and scalability would be much better if there were some
technology used client side that allowed for push.

It's called AJAX.

Okay, it's not actually push, and it does involve blocking inside a
servlet, which we've all just argued against. But that is the modern way
of doing this.

tom

 

[Tom Anderson]

Probably, but I'm not talking about throwing InterruptedException. I'm
talking about restoring the interrupted status.

try {
synchronized(attendee) {
attendee.wait(30000);
}
}
catch (InterruptedException ignored){
Thread.currentThread().interrupt();
}

This is standard procedure for any InterruptedExcpetion that you catch and
don't respond to.

Hmm, i don't recall ever seeing that idiom before.

I see it's mandated here:

http://java.sun.com/javase/6/docs/technotes/guides/concurrency/threadPrimitiveDeprecation.html

The point is that if a thread has its interrupted status set and tries to
start a wait or sleep, it gets an InterruptedException immediately. Thus,
you avoid the problem of waking a thread from one wait/sleep only to have
it go into another one. Makes sense.

The conundrum is knowing when to stop applying it. I assume that at some
point, the interruption can be considered dealt with, and the code that
does that should terminate the interrupt. In this particular case, the
Attendee doesn't know anything about interrupts, so it's not its place to
do that, and it should restore the interrupt status, as you suggest. Good
point.

tom

 

[Mark Space]

The conundrum is knowing when to stop applying it. I assume that at some
point, the interruption can be considered dealt with, and the code that
does that should terminate the interrupt. In this particular case, the
Attendee doesn't know anything about interrupts, so it's not its place
to do that, and it should restore the interrupt status, as you suggest.
Good point.

Java Concurrency in Practice goes into this point in much more detail.
I'd really recommend it to anyone who wants to work in multi-threaded
environments, including Java EE.

Basically, if it's not your thread (and here it's not, the thread is
created and owned by the container) we don't know what the owner of the
thread wants to do on interrupt. So we restore the interrupt and
continue on as quickly as possible.

The rest of the OP's code is fine. He finishes up the response and
returns it to the container. The container can decide whether to send
the result, send some other status, abort the whole thing altogether,
continue like nothing happened, etc.

 

[Tom Anderson]

Java Concurrency in Practice goes into this point in much more detail.
I'd really recommend it to anyone who wants to work in multi-threaded
environments, including Java EE.

Basically, if it's not your thread (and here it's not, the thread is
created and owned by the container)

I'm not totally happy about this idea of thread ownership, but never mind.

we don't know what the owner of the thread wants to do on interrupt.
So we restore the interrupt and continue on as quickly as possible.

Why does the container/owner get to decide what to do on an interrupt?
Sure, if it generated it, then it should, but what if some other code did?
Does it get to handle it anyway? Does that mean that code that isn't the
owner/container shouldn't generate interrupts?

My philosophy (the one i came up with after your previous post!) is that
code which knows how to handle an interrupt is allowed to consume it, even
if it's not the owner, container, top level, etc. But it must only do this
if it *knows* the interrupt is one that it should handle. If it isn't, it
should pass it up.

tom

 

[Mark Space]

My philosophy (the one i came up with after your previous post!) is that
code which knows how to handle an interrupt is allowed to consume it,
even if it's not the owner, container, top level, etc. But it must only
do this if it *knows* the interrupt is one that it should handle. If it
isn't, it should pass it up.

I think the upshot is that you don't know what the interrupt policy of a
thread is if you aren't the owner. (Unless of course it's documented
somewhere.) Thus, only interrupt threads that you created.

Look at it this way. Tomcat likely maintains a pool of threads to
execute any incoming requests. That thread might be executing your
servlet, but it could just as easily execute someone else's servlet, or
some other bit of internal Tomcat code for that matter. Any given
thread could be executing your servlet one request and then execute a
totally different servlet from another application the next request. So
what is the interrupt policy of a thread that is now executing someone
else's servlet? I don't think there's any way to tell.

Owning a thread here means (to me) to have complete control over
everywhere it executes. Then you can control what the interrupt policy
of a thread is.

Interrupting a thread is very different from notifying on an object.
You almost always know who owns the object (if you didn't make it, it's
unlikely you have a reference to it) and most people aren't inclined to
notify on objects that they don't own. And it doesn't matter which
thread is executing, they will all react the same way when notify() is
called on an object.

Interrupting, not so much. Threads are lively things that jump around.
Hard to tell what they're really doing at any given point in time.

Incidentally, the only interrupt policy that really makes sense is
cancellation. Thus, if you do get an interrupt, you might assume that
Tomcat is killing the thread. Thus it's important to restore the
interrupt status so that the rest of the execution (after you return) is
also aware that the thread is in an interrupted state.

 

[Danny]

I think the upshot is that you don't know what the interrupt policy of a
thread is if you aren't the owner.  (Unless of course it's documented
somewhere.)  Thus, only interrupt threads that you created.

Look at it this way.  Tomcat likely maintains a pool of threads to
execute any incoming requests.  That thread might be executing your
servlet, but it could just as easily execute someone else's servlet, or
some other bit of internal Tomcat code for that matter.  Any given
thread could be executing your servlet one request and then execute a
totally different servlet from another application the next request.  So
what is the interrupt policy of a thread that is now executing someone
else's servlet?  I don't think there's any way to tell.

Owning a thread here means (to me) to have complete control over
everywhere it executes.  Then you can control what the interrupt policy
of a thread is.

Interrupting a thread is very different from notifying on an object.
You almost always know who owns the object (if you didn't make it, it's
unlikely you have a reference to it) and most people aren't inclined to
notify on objects that they don't own.  And it doesn't matter which
thread is executing, they will all react the same way when notify() is
called on an object.

Interrupting, not so much.  Threads are lively things that jump around.
  Hard to tell what they're really doing at any given point in time.

Incidentally, the only interrupt policy that really makes sense is
cancellation.  Thus, if you do get an interrupt, you might assume that
Tomcat is killing the thread.  Thus it's important to restore the
interrupt status so that the rest of the execution (after you return) is
also aware that the thread is in an interrupted state.

Thanks everyone. I have not had the time to digest everthing said so
far. (Where else can one get such help as you are providing?) Being
from the South of the USA, I really meant to say: thank you all.

As you can see, I am not yet comfortable with Java and even less so
with its multi-threaded world.

When I have digested your replies, I will respond with the depth they
deserve.

Danny

 

[Arne Vajhøj]

It's called AJAX.

Okay, it's not actually push, and it does involve blocking inside a
servlet, which we've all just argued against. But that is the modern way
of doing this.

The refreshing JavaScript is often done as or called AJAX today.

And it does not required blocking - just the huge overhead of poll.

Java applets, Flash etc. can support real push.

Arne

 

[Daniel Pitts]

The refreshing JavaScript is often done as or called AJAX today.

And it does not required blocking - just the huge overhead of poll.

Java applets, Flash etc. can support real push.

Arne

There is a concept called "comet" which is like AJAX, but uses a
blocking request. The servlet "response" only once there is an event
that the client should know about.