----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/#review1501 -----------------------------------------------------------
trunk/qpid/cpp/bindings/qmf2/examples/cpp/event_driven_list_agents.cpp <https://reviews.apache.org/r/1557/#comment3441> When using the notifier to control your program's main loop, nextEvent should be called with a timeout of Duration::IMMEDIATE so that it cannot block. trunk/qpid/cpp/src/qmf/AgentSession.cpp <https://reviews.apache.org/r/1557/#comment3442> 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. trunk/qpid/cpp/src/qmf/ConsoleSessionImpl.h <https://reviews.apache.org/r/1557/#comment3444> You need to pay attention to mutex protection of eventNotifiers. There's a convention used (see enqueueEvent and enqueueEventLH) whereby functions that assume that a lock is held have their names suffixed by "LH" (lock held). alertEventNotifiers is (I claim) always going to be called with the session lock held and should be a "LH" function. {add,remove}EventNotifier are not "LH" functions but they must acquire the lock before touching eventNotifiers. Not doing so will cause difficult-to-debug problems down the road. trunk/qpid/cpp/src/qmf/EventNotifierImpl.h <https://reviews.apache.org/r/1557/#comment3443> Unfortunately, int handles are Posix-specific. The strategy of having a common interface and a Posix-specific body is not completely valid because notifiers need to have different APIs for Posix and Windows. I'm afraid that this distinction needs to be exposed in the API. trunk/qpid/cpp/src/qmf/PosixEventNotifierImpl.cpp <https://reviews.apache.org/r/1557/#comment3445> 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. trunk/qpid/cpp/src/tests/EventNotifierTest.cpp <https://reviews.apache.org/r/1557/#comment3446> I'd like to suggest that these test cases be moved into cpp/src/tests/Qmf2.cpp trunk/qpid/cpp/src/tests/EventNotifierTest.cpp <https://reviews.apache.org/r/1557/#comment3447> assert(true) is a no-op and is not needed. trunk/qpid/cpp/src/tests/EventNotifierTest.cpp <https://reviews.apache.org/r/1557/#comment3448> Don't use assert in test cases, use BOOST_CHECK_EQUAL, BOOST_CHECK_THROW, etc. See Qmf2.cpp for examples. - Ted 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 > >
