cometd.js, _connectResponse and ending a batch

[Greg]

I was looking through the code to try and work out the cause of some
odd behaviour I was seeing. and whilst doing so found what I think
could be an error (FWIW, the "odd behaviour" was nothing to do with
the client).

Currently, in _connectResponse in cometd.js the client code ends the
batch mode (line 798, in RC0) with a call to _endBatch(true); so that
any outstanding messages are sent with the next long poll.

However, I can see two issues with that;
1) The caller may have called more than one startBatch(), so there's
no guarantee that the batched messages will be sent.

2) If the caller is batching up a number of messages, and the
_connectResponse is received in the middle of that, the benefits of
the batch are lost; consider
a) caller does startBatch()
b) caller does a publish( messageA )
c) _connectResponse fires, does an _endBatch() which sends messageA
d) caller does a publish( messageB ) - this is sent immediately as
there is no batch open
e) caller does a publish( messageC ) - this is queued as we're using
the 2 connections
f) caller does an endBatch() - this has no effect, as it's already
been ended (step c)

This means that messageC ends up being delayed longer than the client
would anticipate. I think it should be easy to fix, though. Simply
replace line 798 - _endBatch(true); - with ...

var _saveBatch = _batch; // Save the current batch level
_batch = 1; // reset the batch level so that any batched messages are
sent now
_endBatch(true); // send the messages
_batch = _saveBatch; // and restore the original batch level

This means that 1) messages are sent, regardless of the number of
batches the caller has started, and 2) any subsequent messages are
batched together as the caller expects (in the above example, messageB
& C are batched together again).

Am I right, or have I missed something?

Thanks,

Greg

[Simone Bordet]

Hi,

On Sat, Nov 21, 2009 at 15:32, Greg <greg.d...@gmail.com> wrote:
> I was looking through the code to try and work out the cause of some
> odd behaviour I was seeing. and whilst doing so found what I think
> could be an error (FWIW, the "odd behaviour" was nothing to do with
> the client).
>
> Currently, in _connectResponse in cometd.js the client code ends the
> batch mode (line 798, in RC0) with a call to _endBatch(true); so that
> any outstanding messages are sent with the next long poll.

Not exactly. They're sent immediately via the non-long poll connection.

I think you are missing the one-threaded constraint of the JS environment.
There is no way that c) above executes in the middle of a user
operation; it's executed *sequentially* to it.

Imagine the user clicks on a button to trigger this:

function _handler()
{
cometd.startBatch();
cometd.publish('/my/channel', {data: 'a'});
cometd.publish('/my/channel', {data: 'b'});
cometd.publish('/my/channel', {data: 'c'});
cometd.endBatch();
}

Function "_handler" is executed atomically.
If, meanwhile, the server returns the long poll, any callback in the
Cometd JS implementation is executed by the JS interpreter either
before _handler, or after it, but not concurrently.

You can do horrible tricks with the only purpose of breaking the
Cometd implementation, but I don't think we should guard against
those, unless proven they're a viable use case.

Simon
--
http://bordet.blogspot.com
---
Finally, no matter how good the architecture and design are,
to deliver bug-free software with optimal performance and reliability,
the implementation technique must be flawless. Victoria Livschitz

[Greg Thomas]

2009/11/23 Simone Bordet <simone...@gmail.com>

Hi,

On Sat, Nov 21, 2009 at 15:32, Greg <greg.d...@gmail.com> wrote:
> Currently, in _connectResponse in cometd.js the client code ends the
> batch mode (line 798, in RC0) with a call to _endBatch(true); so that
> any outstanding messages are sent with the next long poll.

Not exactly. They're sent immediately via the non-long poll connection.

 

OK, I'm surprised as that; as we've just received the response from a long-poll, I'd
assumed that the next thing that will go out is another long poll. It strikes me as
wasteful to send out a long-poll followed immediately by a non-long poll. Anyway,
it's of no odds to the matter at hand.

...

You can do horrible tricks with the only purpose of breaking the
Cometd implementation, but I don't think we should guard against
those, unless proven they're a viable use case.

JS is single threaded, yes, but it doesn't mean that you have to carry out
a startBatch/endBatch pair in a single thread. For example, consider an online
shopping basket. The client opens a batch. When a users adds an
item to the basket, it's sent as publish'd to the server. When the client starts
the checkout process, the batch is ended. OK, I probably wouldn't implement
it that way myself, but I wouldn't consider it a horrible trick.

Given that the fix is relatively simple, it seems a shame not to support it. But if
it's not going to be supported, then at the very least the limitation should be
documented - it's not currently mentioned at http://cometd.org/node/55

Greg

[Simone Bordet]

Hi,

On Mon, Nov 23, 2009 at 19:52, Greg Thomas <greg.d...@gmail.com> wrote:
> 2009/11/23 Simone Bordet <simone...@gmail.com>
>>
>> Hi,
>>
>> On Sat, Nov 21, 2009 at 15:32, Greg <greg.d...@gmail.com> wrote:
>> > Currently, in _connectResponse in cometd.js the client code ends the
>> > batch mode (line 798, in RC0) with a call to _endBatch(true); so that
>> > any outstanding messages are sent with the next long poll.
>>
>> Not exactly. They're sent immediately via the non-long poll connection.
>
>
> OK, I'm surprised as that; as we've just received the response from a
> long-poll, I'd
> assumed that the next thing that will go out is another long poll. It
> strikes me as
> wasteful to send out a long-poll followed immediately by a non-long poll.

Sure, this could be optimized, though it may complicate the
implementation a bit.

> JS is single threaded, yes, but it doesn't mean that you have to carry out
> a startBatch/endBatch pair in a single thread. For example, consider an
> online
> shopping basket. The client opens a batch. When a users adds an
> item to the basket, it's sent as publish'd to the server. When the client
> starts
> the checkout process, the batch is ended. OK, I probably wouldn't implement
> it that way myself, but I wouldn't consider it a horrible trick.

Well it's not what batches have been designed for.
They are not meant to span more than one context of execution.

I would be terribly deluded by my online shop if I choose 20 books,
put them in my basket (or thinking so), then my browser crashes and
whoops, my basket is gone.

> Given that the fix is relatively simple, it seems a shame not to support it.

I have not looked into your suggestion in detail, since IMHO your
premises are wrong.
I still have not yet a valid use case; your online store above seems a
bit too stretched to me (and you would not do that as well).
Plus, I am a little nervous making this change now that we're prepping
for 1.0 final.

> But if
> it's not going to be supported, then at the very least the limitation should
> be
> documented - it's not currently mentioned at http://cometd.org/node/55

Will do that.

BTW, in your first email you said you had a problem and you thought it
was related to this.
You still have a problem, or this has been solved by your other thread
on cometd-users ?

Thanks,

[Greg Thomas]

2009/11/23 Simone Bordet <simone...@gmail.com>

Plus, I am a little nervous making this change now that we're prepping
for 1.0 final.

That's probably the most relevant thing, yes :)
 

> But if
> it's not going to be supported, then at the very least the limitation should
> be
> documented - it's not currently mentioned at http://cometd.org/node/55

Will do that.

Thanks
 

BTW, in your first email you said you had a problem and you thought it
was related to this.
You still have a problem, or this has been solved by your other thread
on cometd-users ?

No, I'm all sorted. It was a bug in the server I was using (http://code.google.com/p/aspcomet/),
that's now resolved. I was poking around in the code trying to figure out what was causing my
problem. I found it, but whilst doing so noticed this (unrelated) 'issue'.

Thanks for the help, and clearing things up for me,

Greg