Add storage of Queues and Exchanges to QAmqpClient. #27

sjlongland · 2015-04-13T22:51:55Z

Rather than handing out "new" exchange and queue objects, we conserve memory and channel usage by keeping a list of existing objects.

If an object is asked for by name, we look for the existing object of that name first before declaring a new one. When a queue is asked for without a name, we wait until it is declared (thus given a name by the broker) before we store it.

For exchanges, all exchanges are stored by name: NULL QStrings are converted to an empty string to avoid having duplicate references to the "nameless" exchange.

Just discovered you *CAN* delete them yourself safely, and one of the tests does. So best to not assume they'll get deleted by other parties. QPointer takes care of this problem for us.

Since there will be probably many requests for that one. If we're passed in QString() instead of QString(""), we overwrite that with the latter to ensure the name field is never NULL.

How on earth did I expect that to work? Morning cup of tea hasn't kicked in yet, of course I can't assign a const QString passed by reference. Fix egregious PEBKAC-caused compile error.

On 'destroyed', do a clean up of our exchanges and queues.

Something in the back of my mind had me thinking iterator.value was a field not a function. No idea why. Fixed now.

- DON'T rely on sender() - DON'T expect to be able to cast the QObject to a QAmqpChannel and have name() work. This isn't as efficient as I'd like, but it doesn't crash either.

Maybe my laptop is too quick, but it seems the exchange hangs around a little too long for this test to pass.

Same issue, maybe with the state machine the QAmqpQueue takes a little longer to disappear.

mbroadst · 2015-04-14T02:52:44Z

src/qamqpchannelhash.cpp

+*/
+QAmqpChannel* QAmqpChannelHash::get(const QString& name) const
+{
+    if (name.isNull())


|| name.isEmpty(), that translates to "is the QString default constructed (QString()), OR is it an empty string ''"

mbroadst · 2015-04-14T12:03:34Z

okay! I think those are all my comments 😈 it seems you're still not passing a test, but it might be invalid at this point?

mbroadst · 2015-05-01T03:23:33Z

@sjlongland ping. I think that one test can be disabled considering your changes here. Then we can at least merge this in (sorry this has been somewhat trying)

sjlongland · 2015-05-01T03:46:50Z

Hi Matt,
On 01/05/15 13:23, Matt Broadstone wrote:

@sjlongland https://github.com/sjlongland ping. I think that one test
can be disabled considering your changes here. Then we can at least
merge this in (sorry this has been somewhat trying)

No problems there. I've just been flat out lately, in addition to the
C based code I've been writing there's a Python-based counterpart
using pika that I'm also in the process of writing.

It was experimenting with QAMQP that told me I had problems in my Python
code, so I'm working on eliminating those. I've taken the ideas used in
these QAMQP branches and applied those to a wrapper around Pika Tornado,
using Fysom as a state machine.

The same concepts should work for QAMQP with QStateMachine. I do intend
to get back to the C code since the performance felt orders of

magnitude faster.

Stuart Longland (aka Redhatter, VK4MSL)

I haven't lost my mind...
...it's backed up on a tape somewhere.

When we initiate a Close, we get a CloseOk in response and on receipt of the CloseOk, we mark the channel as closed and emit a signal to notify everyone else. Great. Now what happens when the server closes the channel? We emit a CloseOk, then… nothing. We do nothing. This fixes this problem, moving the notification/marking logic to a new function and calling that on both Close (sent from server) and CloseOk.

If the server closes the channel on us, re-open it before removing the exchange.

sjlongland · 2015-05-02T10:48:49Z

Okay, that's just fixed up the failing test case. Now to address the code style issues. :-)

sjlongland · 2015-05-02T11:19:10Z

That should have addressed the remaining issues, if I've missed anything, let me know. :-)

sjlongland · 2015-05-24T20:43:06Z

Out of interest, I just had a closer look at QStateMachine. One problem I see is there's no way to directly trigger a transition from a method other than to have declared a signal and to then emit that signal.

e.g. to use an existing example in Fysom (a Python state machine):

fsm = Fysom({'initial': 'green',
             'events': [
                 {'name': 'warn', 'src': 'green', 'dst': 'yellow'},
                 {'name': 'panic', 'src': 'yellow', 'dst': 'red'},
                 {'name': 'panic', 'src': 'green', 'dst': 'red'},
                 {'name': 'calm', 'src': 'red', 'dst': 'yellow'},
                 {'name': 'clear', 'src': 'yellow', 'dst': 'green'}],
             'callbacks': { … }})

fsm.panic(msg='killer bees')
fsm.calm()
fsm.clear()

— https://github.com/mriehl/fysom

There, each event gets its own method on the state machine object, which can then be called to signal events to the state machine. It seems QStateMachine requires that the events be signals or keyboard/mouse events.

We've got a couple of options:

Carry on with the homebrew enum-based state machine (not as elegant, but does what we want)
Create special private signals that are for the state machine's benefit.
Create a QAbstractState subclass that presents the transition as a method which we can call to transition between states.

mbroadst · 2015-06-08T13:22:38Z

@sjlongland I'm really sorry, I've been incredibly busy and this got away from me (shame on me for pinging you in the first place only to disappear 😄). I agree with your wrt QStateMachine, I've often found it lacking for anything but automation of Qt gui states - however I did come to a similar conclusion that this -might- be possible with QAbstractStates. I think for now perhaps the best way to go about it is to continue to use the private enums (unless you're working on something currently)

mbroadst · 2015-06-08T13:25:35Z

src/qamqpclient_p.h

@@ -100,6  101,12 @@ class QAMQP_EXPORT QAmqpClientPrivate : public QAmqpMethodFrameHandler
    QAMQP::Error error;
    QString errorString;

+    /*! Exchange objects */
+    QAmqpChannelHash exchanges;


is there a reason you have two hashes here? QAmqpChannelHash is already built for abstracting the channels themselves, so you can store them all in a single hash.

What happens if a user declares an exchange named foo, and declares a queue named foo? I'm not sure if that's against the AMQP specs but if permitted, then we'd have two different objects with the same name.

This handles that possibility in the cleanest way I can think of. I figured it was easier to do this than to try and find out if the other was forbidden. :-)

Add storage of Queues and Exchanges to QAmqpClient.

sjlongland · 2015-06-08T22:03:03Z

On 08/06/15 23:22, Matt Broadstone wrote:

@sjlongland https://github.com/sjlongland I'm really sorry, I've
been incredibly busy and this got away from me (shame on me for
pinging you in the first place only to disappear 😄).

This is fine, I've been flat out on the python side of things, we're
still umming and arring about whether we do a move to C for the
high-performance stuff and leave Python for things that are too hard in
C , or whether we stay purely Python based.

Right now the codebase I'm working on is officially pure Python with the
C code being a proof-of-concept, but this may change as we're needing
to squeeze a little more oomph out of little industrial computers with
128MB RAM: Python is getting a bit too chunky.

I agree with your wrt QStateMachine, I've often found it lacking for
anything but automation of Qt gui states - however I did come to a
similar conclusion that this -might- be possible with
QAbstractStates. I think for now perhaps the best way to go about it
is to continue to use the private enums (unless you're working on
something currently)

The other option might be we port the concepts over from Jake Gordon's
finite state machine¹ to Qt and that would give us a pretty robust
non-GUI state machine.

The existing QStateMachine could also be used, we'd just have to come
up with our own state transition class that can be triggered by a method.

Regards,

Stuart Longland (aka Redhatter, VK4MSL)

I haven't lost my mind...
...it's backed up on a tape somewhere.

http://codeincomplete.com/posts/2014/3/15/javascript_state_machine_v2_3_0/

sjlongland added 11 commits April 13, 2015 08:26

QAmqpClientPrivate: Store pointers to named queues and exchanges.

ae77ff5

QAmqpClient: Use QPointer to store exchanges and queues.

fd4eaef

Just discovered you *CAN* delete them yourself safely, and one of the tests does. So best to not assume they'll get deleted by other parties. QPointer takes care of this problem for us.

QAmqpClient: Store the nameless exchange too.

2622a48

Since there will be probably many requests for that one. If we're passed in QString() instead of QString(""), we overwrite that with the latter to ensure the name field is never NULL.

QAmqpClient: Fix attempt to assign const QString

bca3f5f

How on earth did I expect that to work? Morning cup of tea hasn't kicked in yet, of course I can't assign a const QString passed by reference. Fix egregious PEBKAC-caused compile error.

QAmqpClient: Add garbage collection slot.

6599fdb

On 'destroyed', do a clean up of our exchanges and queues.

QAmqpClient: Fix 'does not have class type' error.

6732743

Something in the back of my mind had me thinking iterator.value was a field not a function. No idea why. Fixed now.

QAmqpChannelHash: Container for storing channels.

beb89b3

QAmqpClient: Re-factor using QAmqpChannelHash.

ae4f1b3

QAmqpChannelHash: Tweak channelDestroyed.

31f90a1

- DON'T rely on sender() - DON'T expect to be able to cast the QObject to a QAmqpChannel and have name() work. This isn't as efficient as I'd like, but it doesn't crash either.

QAmqpExchange tests: Wait for destroyed signal.

f79e96f

Maybe my laptop is too quick, but it seems the exchange hangs around a little too long for this test to pass.

QAmqpQueue Tests: Wait for destroyed signal.

568dc67

Same issue, maybe with the state machine the QAmqpQueue takes a little longer to disappear.

mbroadst reviewed Apr 14, 2015
View reviewed changes

Stuart Longland added 2 commits May 2, 2015 20:41

QAmqpExchange redeclaration test: Re-open a closed channel.

cd655d3

If the server closes the channel on us, re-open it before removing the exchange.

Stuart Longland added 5 commits May 2, 2015 20:52

QAmqpChannelHash: Coding style fixes.

3e5cf05

QAmqpChannelHash: Add contains function

c171b5c

QAmqpChannelHash: Store empty/null strings as null strings.

e432248

QAmqpChannelHash: Substitute static_cast for qobject_cast

60f680a

QAmqpClient: Substitute static_cast for qobject_cast

fd1b19b

mbroadst reviewed Jun 8, 2015
View reviewed changes

mbroadst added a commit that referenced this pull request Jun 8, 2015

Merge pull request #27 from vrtsystems/state-machine-client

73d80de

Add storage of Queues and Exchanges to QAmqpClient.

mbroadst merged commit 73d80de into mbroadst:master Jun 8, 2015

sjlongland deleted the state-machine-client branch June 8, 2015 21:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add storage of Queues and Exchanges to QAmqpClient. #27

Add storage of Queues and Exchanges to QAmqpClient. #27

sjlongland commented Apr 13, 2015

mbroadst Apr 14, 2015

mbroadst commented Apr 14, 2015

mbroadst commented May 1, 2015

sjlongland commented May 1, 2015

sjlongland commented May 2, 2015

sjlongland commented May 2, 2015

sjlongland commented May 24, 2015

mbroadst commented Jun 8, 2015

mbroadst Jun 8, 2015

sjlongland Jun 8, 2015

sjlongland commented Jun 8, 2015

Add storage of Queues and Exchanges to QAmqpClient. #27

Add storage of Queues and Exchanges to QAmqpClient. #27

Conversation

sjlongland commented Apr 13, 2015

mbroadst Apr 14, 2015

Choose a reason for hiding this comment

mbroadst commented Apr 14, 2015

mbroadst commented May 1, 2015

sjlongland commented May 1, 2015

magnitude faster.

sjlongland commented May 2, 2015

sjlongland commented May 2, 2015

sjlongland commented May 24, 2015

mbroadst commented Jun 8, 2015

mbroadst Jun 8, 2015

Choose a reason for hiding this comment

sjlongland Jun 8, 2015

Choose a reason for hiding this comment

sjlongland commented Jun 8, 2015

Regards,