On Tue, 18 Nov 2008, Danny wrote:
> 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
--
If goods don't cross borders, troops will. -- Frédéric Bastiat
|
Similar Threads
