> On 2011-08-17 16:34:38, Ted Ross wrote: > > trunk/qpid/cpp/src/qmf/AgentSession.cpp, line 1095 > > <https://reviews.apache.org/r/1557/diff/1/?file=33107#file33107line1095> > > > > The logic here is incorrect. It is not necessarily the case that the > > reception of a message from the AMQP session will result in the creation of > > an event in the QMF session. Conversely, there are cases where QMF events > > will be generated when there were no AMQP messages received (e.g. Agent > > age-out due to lack of heartbeats received). > > > > This is not the right place to kick the notifiers. > > > > The notifier should be enabled when the event queue goes from empty to > > non-empty and should be disabled when the event queue becomes empty.
I've moved the call to alertEventNotifiersLH() to the enqueueEventLH() method in both Session types. There also needs to be a spot where an event should fire when the event queue is empty. I've placed a call within nextEvent() that does that both after popping the next message off of the queue and, if the queue is empty, in an else statement. > On 2011-08-17 16:34:38, Ted Ross wrote: > > trunk/qpid/cpp/src/qmf/PosixEventNotifierImpl.cpp, line 36 > > <https://reviews.apache.org/r/1557/diff/1/?file=33114#file33114line36> > > > > We've had a problem in the past with a similar pattern: If the pipe > > ever fills up (the capacity is OS-specific), the write call may misbehave, > > causing deadlocks. > > > > Also, if the pipe ever has more than BUFFER_SIZE characters in it, > > setting readable to False will have no effect. > > > > Here's a good test case that will fail: > > call setReadable(true) 11 times; > > call setReadable(false) once; > > Verify that the fd is unreadable. > > > > > > I suggest keeping a boolean state that tracks the readability of the > > socket. If the call to setReadable will not result in a state change, > > don't do anything. This guarantees that the pipe will never have more than > > one character in it at any time. > > > > Since this call is always invoked under the session's lock, and a > > notifier can have exactly one session, there is no need to add a mutex to > > protect the stored state. Done. Now the method will check: * if we're readable and the new state is readable==false then pull the byte out, else * if we're not readable and the new state is readable=true then write a byte - Darryl ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/#review1501 ----------------------------------------------------------- On 2011-08-17 14:07:58, Darryl Pierce wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1557/ > ----------------------------------------------------------- > > (Updated 2011-08-17 14:07:58) > > > Review request for qpid, Kenneth Giusti, michael goulish, and Ted Ross. > > > Summary > ------- > > Provides a new method for providing notification to an interested party when > new messages are received. > > The EventNotifier class can be associated with either a console or agent > session. The object provides a file descriptor which then becomes readable > when there are messages to be processed. > > This implementation only supports Posix. There is some work necessary to get > a Windows implementation in place. > > > Diffs > ----- > > trunk/qpid/cpp/bindings/qmf2/examples/cpp/Makefile.am 1158370 > trunk/qpid/cpp/bindings/qmf2/examples/cpp/event_driven_list_agents.cpp > PRE-CREATION > trunk/qpid/cpp/include/qmf/AgentSession.h 1158370 > trunk/qpid/cpp/include/qmf/ConsoleSession.h 1158370 > trunk/qpid/cpp/include/qmf/EventNotifier.h PRE-CREATION > trunk/qpid/cpp/src/CMakeLists.txt 1158370 > trunk/qpid/cpp/src/qmf.mk 1158370 > trunk/qpid/cpp/src/qmf/AgentSession.cpp 1158370 > trunk/qpid/cpp/src/qmf/AgentSessionImpl.h PRE-CREATION > trunk/qpid/cpp/src/qmf/ConsoleSession.cpp 1158370 > trunk/qpid/cpp/src/qmf/ConsoleSessionImpl.h 1158370 > trunk/qpid/cpp/src/qmf/EventNotifier.cpp PRE-CREATION > trunk/qpid/cpp/src/qmf/EventNotifierImpl.h PRE-CREATION > trunk/qpid/cpp/src/qmf/EventNotifierImpl.cpp PRE-CREATION > trunk/qpid/cpp/src/qmf/PosixEventNotifierImpl.cpp PRE-CREATION > trunk/qpid/cpp/src/tests/EventNotifierTest.cpp PRE-CREATION > trunk/qpid/cpp/src/tests/Makefile.am 1158370 > > Diff: https://reviews.apache.org/r/1557/diff > > > Testing > ------- > > An example agent takes the existing list_agents and uses an EventNotifier to > respond to incoming messages rather than blocking on the > ConsoleSession.nextReceiver() API. > > A unit test verifies that the file handle provides by the EventNotifier type > is properly updating on incoming messgaes. > > > Thanks, > > Darryl > >
