qpid-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alan Conway <acon...@redhat.com>
Subject Re: 'client' APIs again
Date Tue, 12 Aug 2014 09:25:33 GMT
On Mon, 2014-08-11 at 16:47 +0100, Gordon Sim wrote:
> Thanks very much for the comments, Alan, some responses inline...
> 
> On 08/11/2014 03:01 PM, Alan Conway wrote:
> > Naming: I hate things called *_tools, *_utils etc. We should find a more
> > descriptive name. Not foo or fred or whatchymacallit or dingdong or
> > doofer either. Ideally these classes might all just end up in the proton
> > package, or maybe as a subpackage like proton.events or proton.reactor.
> 
> I personally think they should (generally) be kept distinct from the 
> current proton package to make it clear that they are additive and 
> optional. A subpackage would be ok, I just didn't want to mix this with 
> dependent edits to the proton tree itself to begin with. It may even be 
> that not all the additional pieces belong in the same package.

Fair enough. I like the sub-package but it needs to be a little more
descriptive than proton.utils. proton.reactor is growing on me a bit.
proton.events is not so bad - it's all about handling a proton.Event
after all. 

> > Runtime is a bit vague as a name as well though I haven't got a better
> > idea.
> 
> Yes, this was unpopular with other I spoke to also. I reverted to my 
> original name - Container - which isn't ideal either. Any suggestions 
> from other gratefully considered.

Its funny how often we run into this problem in this business. "Object",
"Entity", "Thing", "Class", "Type",
"IHaveToCallItSomethingButIReallyDontWantToBeSpecific" :)

Looking at the code, Runtime does nothing but add connect and accept to
SelectLoop, and SelectLoop is basically an Event pump. I get that we
need to allow other forms of event pump in future, e.g. EPollLoop or
whatever.

So I would first suggest changing the name to

 EventLoop { connect(); accept(); run(); }

Since this is really just a "fully functional" general event loop, based
on some concrete type of event pump implementation (e.g. SelectLoop)

class EventLoop:
    def __init__self(self, handlers=None, PumpClass=SelectLoop):
        self.handlers = handlers or [ default_handlers...]
        self.loop = PumpClass(Events(ScopedDispatcher(), *handlers)

Note that as it stands the current Runtime/SelectLoop contract is not
really suitable for adding new types of pump since RunTime.connect knows
about Selectables, but that contract is something for integrators or
advanced users adding new types of pump so lets get the general user
interface right first.

I did also consider flipping it and making EventLoop an abstract base
for SelectLoop and future pumps, but I'm not sure that helps anything. I
like a separate class that is the end user interface and entirely
separate classes for implementation details.

--- python tips (apologies if obvious these are things I only figured
out recently)

1. don't use import *. E.g. use "from proton import Events,...."
import * makes it easier to write but harder to read since you have to
consult other files to figure out where things come from.

2. NEVER use map/list literals as keyword argument defaults!

def this_is_bad(handlers=[]): ....
 
because the list/map is created at module load time NOT each time the
function is called!!! Since lists & maps are mutable the default can
easily be inadvertently changed . Very hard to debug! So do

def this_is_better(handlers=None):
  self.handers = handlers or [] # OR
  if self.handlers is None: ...

This has the added bonus of being able to differentiate between an
argument that was not supplied (None) or supplied as an empty list ([])
if that matters.

> 
> >
> > Get rid of Runtime.DEFAULT. Developers who want a global instance should
> > make their own and be responsible for its scope:
> >
> >      DEFAULT = Runtime()
> >
> > We definitely should not provide a global in the library (I have
> > suffered horribly in the past from this mistake. Seems harmless at first
> > but creates nightmarish startup & shutdown ordering issues.)
> 
> Yes, I was coming to that conclusion myself...
> 
> > I'd modify the Runtime constructor a little:
> >
> > class Runtime:
> >      def __init__(self, handlers=None):
> >          if handlers is None: handlers = [FlowController(10)]
> >          self.handlers = handlers
> > That make it possible to be clear about whether you want default
> > handlers Runtime() or no handlers Runtime([])
> 
> Good idea, I'll do that and it's a nicer solution to the defaults than a 
> global.
> 
> > Example ordering: I would put the direct example first. People want to
> > see something work right away, it is annoying if the first thing you
> > have to do is set up some extra software. I think the broker example
> > second would be fine, even if it is a line shorter.
> 
> I do see your point and I'll chew this over a bit.
> 
> > Waiting for subscriptions with direct connections: one of the things
> > that bit me hard and often with proton is waiting for things to be
> > ready. In particular if you create a receiver & sender on separate
> > connections and send immediately you have a race condition where the
> > message will sometimes be lost because the sender sent it before the
> > subscription was actually active. This is quite infuriating to debug.
> 
> Yes (unless of course you use some sort of 'broker' that could perhaps 
> 'queue up' messages it received until there was a subscriber to which 
> they could be forwarded!)

I think it's important (and you already have it:) to highlight the
differences between using a broker and doing direct messaging. This is
one of the key direct-messaging gotchas to be aware of.

> 
> > You dodge the issue by using the same connection for sender & receiver
> > which is not the case for any but the most trivial example.
> > I suggest you face this head on:
> 
> That's a fair point. I've received other feedback that the examples were 
> not 'reactive' enough - too much done before the event loop is entered, 
> not enough done within the loop itself - which fits in with this.
> 
> I've already made some changes that address the lack of reactivity by 
> creating the sender and receiver only when the connection is ready and 
> sending only when credit has been issued. I could perhaps modify this 
> further to have the sender created only once the receiver is ready.
> 
> The request-response example shows that already of course, since the 
> receiver's address, generated by the broker, is required before sending 
> the first message.

> One of the goals is to emphasise that simple things can be done simply.

Like Einstein said: Make everything as simple as possible _but no
simpler_

> In my first attempt I fell in to the trap of trying to make the first 
> example so simple that it lost any real illustrative value. However it 
> is 'hello world' and I don't want to overcomplicate it either, 
> especially if the issues could be reasonably dealt with in subsequent 
> examples... just need to find the right balance and all the feedback 
> helps so thanks again!
> 
> > the direct example should show how to
> > create a receiver on one connection, wait till it is active, then send
> > on a different connection.
> 
> The direct example only actually creates one link (it's a sender from 
> the "client's" perspective and a receiver from the "server's"). I quite 
> like that aspect of it so I think your comment is perhaps more 
> applicable to the non-direct i.e. brokered case.

Nope, you are missing the point, let me expand.

There is a very common pattern in distributed systems.

1. Send notification that you want to receive messages at an address.
2. WAIT for the message receiving machinery to be _ready to receive_
3. Advertise the address to senders
4. Senders send, receivers receive everybody is happy.

If 1 is asynchronous and you don't do 2 then you have lost messages and
confusion all around. 

A broker solves the problem for you by queuing messages so it doesn't
matter if step 4 happens before 2 completes.

A hello-world demo over a single connection hides the problem because
the subscribe and send are serialized over the connection. This is never
the case in a real-world system. 

The qpid.messaging API solves this problem by allowing the user to make
a synchronous subscribe call that does 1 and 2 together.

Sending a subscribe request in this new API (or in Messenger for that
matter) *only does step 1*. We need to show people step 2 as well. 

The new API is perfect as we have access to the subscription-is-ready
event. I want a demo that shows how to use it and explains why. It can
be an additional demo if we want to keep the super simple one, although
I think a 2 connection demo that waits for the subscription would not be
so complex. Maybe I should stop whining and write it.

This is not just a demo/test concern. Yes there are patterns where this
concern does not arise:
- brokered store and forward
- systems that are ok with lost messages
- systems that have a quiet period where they can set things up before
clients become active.

However lots of systems need dynamic creation of addresses while the
system is busy - RPC style systems often fall into this category. Any
time anything can request a "temporary" address that is not widely
advertised in advance you can have this problem. 

This is also not an abstract concern, it is one of the first things I
hit writing dispatch system tests using Messenger as the client:
- "why did Ted put: 'while messenger.work(0.1): pass' after every
messenger.subscribe?"
- "why do my tests hang and messages vanish all the time?"
- "ohhh THAT's why"

Probably working with dispatch, which makes the business of address
creation and subscription more distributed and slower, makes it more
likely to hit this issue. But the issue is still there. If you're
unlucky enough not to hit it in development you can be sure it will bite
you in production :)


> 
> > Reconnect: needs more connection lifecycle events (connected,
> > heartbeats) but I see you are planning to address this.
> 
> Yes. So far I've been making some updates based on feedback received 
> (examples are all now updated as checked in, though the tutorial text is 
> still lagging behind a little). However my next step was to look at 
> timer driven events (e.g. for heartbeats or reconnect backoff) etc, and 
> some more detailed examples on approaches to reliable messaging.
> 
> > Messages and threads: once received an app will want to dispatch
> > messages based on address and other properties and allocate processing
> > to threads in various ways. This is (should be) orthogonal to the API
> > you are designing here but we need to make sure we have adequate thread
> > safety and don't create limitations in the API that make things
> > difficult.
> 
> I agree that more thought and examples on how to interact with other 
> threads is important. However at the same time, I want to avoid trying 
> to make everything threadsafe.

Agreed.

> 
> > E.g. the app may want to process messages from a single
> > proton connection (i.e. a single event stream) in multiple threads and
> > send reply messages back to the same connection from arbitrary threads.
> 
> The proton connection object is not threadsafe, and I don't think that 
> should change.
> 

Agreed. What I'm getting at is there's a need for more tools (thread
pools, thread safe queues) to allow people to write multi-threaded
message servers that aren't necessarily tied to the threading model of
proton. But I think that should be an add-on not a change to what you
are doing here, so probably a distraction at this point.

> Providing utilities to allow message processing on different threads and 
> have interaction with a given connection coordinated is certainly 
> something I agree should be explored.
> 
> > We should also make it possible for a single thread pool to be used to
> > handle proton and application events.
> >
> > MessagingContext: I found this a bit confusing. I got the impression
> > that its an abstraction over either a session or a connection, but I am
> > not too clear what purpose it serves.
> 
> Yes, it's (mostly) to allow users to ignore sessions unless they 
> explicitly want to control them. (It also provided a convenient place to 
> locate some utility code for creating links in the common cases which is 
> quite verbose with the engine api as it is).
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
> For additional commands, e-mail: users-help@qpid.apache.org
> 



---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org


Mime
View raw message