qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ted Ross" <tr...@apache.org>
Subject Re: Review Request: Provides the EventNotifier type, plus example agent and unit test.
Date Wed, 17 Aug 2011 16:34:38 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message