Messages not broadcast in correct order when format is used

[Bud Schneehagen <buddy@…>]

Format-specific "broadcasts" blocks are running after other broadcasts in the same event. Here is a simple test case.

http://www.cf-ezcart.com/modelgluesamples/helloworld/?event=page.test&requestformat=remote

[boomfish]

OUCH!

This is definitely a serious defect, but the original diagnosis of the cause (related to event types) was incorrect. The actual problem is that messages not tied to a format are always broadcast ahead of format-specific messages, even when that is not the order they are listed in.

Example:

<event-handler name="page.test">
	<broadcasts format="remote">
		<message name="remoteFunctions"/>
	</broadcasts>
	<broadcasts format="html">
		<message name="htmlFunctions"/>
	</broadcasts>
	<broadcasts>
		<message name="eventFunctions"/>
	</broadcasts>
</event-handler>

When the above event handler is executed, the "eventFunctions" message is always broadcast before the other two messages, even when the requestFormat is "remote" or "html".

[boomfish]

Pushed ticket to next milestone.

[cfgrok]

I believe that I have worked out a solution for this issue. The root cause of this problem was the fact that messages, results and views with different request formats were split out into separate storage locations within an event-handler. Messages, for example, would be stored in one or more arrays, each of which was assigned to a structure with a key corresponding to the request format of the message.

At execution time, the message array containing "unformatted" messages would be processed first, followed by the message array matching the current request format (if such an array existed). Therefore, messages without a format would always be broadcast before messages with an explicit format, regardless of their relative order in the configuration file.

I am attaching a couple of patches to this ticket -- the first contains a set of unit tests targeting this issue, and the second will apply my proposed fix.

This patch corrects the problem by using a single "container" variable for each of the three entities (arrays for messages and views, and a structure for results), and adding a new "format" property to the message, result and view objects. At execution, a conditional statement will run the message/result/view if the format property is an empty string, or it matches the current request format.

My only concern with this approach is whether the string parsing adds a measurable amount of overhead to each executing request, which would obviously be undesirable. (I noted that the original implementation included comments such as: "Code repeated for format, if necessary, to avoid string parsing - this is a per-request invocation!")

Any comments or feedback would be much appreciated.

[cfgrok]

Unit tests for request format execution order problem

[cfgrok]

Proposed solution for request format execution order problem

[boomfish]

When I was looking at this issue I thought of a different approach that keeps the per-request time overhead low in exchange for some start-up time overhead and a small increase in memory usage.

Instead of merging the arrays for each format, we keep the arrays separate but add the "unformatted" elements across all the arrays. The elements are all value objects so we'd only be copying references, keeping the memory overhead small.

When a formatted element is first encountered and an array for that format is created, copy all the elements from the "unformatted" array to the new array.

At run-time, select one array for the request: either a format array (if one matches the request format) or the "unformatted" array. Each array would contain all the applicable elements in the correct order.

I can create a patch for this over the weekend if you're interested.

[cfgrok]

I had a somewhat similar idea as well, which was to eliminate the unformatted arrays, and to add the unformatted elements to each of the formatted arrays, forcing the creation of arrays for the default request format. I had mostly worked out a patch implementing this strategy, but there are some significant issues with this approach, primarily the fact that the entire list of possible request formats would need to be known before module loading, which in turn required some contortions on initialization requests. This being the case, your suggestion is a much more effective solution.

If you'd like to create another patch for this ticket, feel free. I'm all for going with whatever fix works best, so if your idea turns out to be the better one, then we should probably implement it. That said, I performed some load testing tonight, and I'm not currently convinced that the attached patch causes any decrease in real-world performance.

The tests did indicate a higher average request time, but we're talking about the difference between 41 and 44 ms. Yes, that is an increase of 7% or so, but obviously there would be no perceptible change from an actual user's perspective, and I'm not at all sure that it's statistically significant. (I can send you the load test parameters and results if you'd like.)

I guess at this point I'd probably vote for implementing this patch, as it's already complete, has passed all unit tests, and doesn't appear to have a measurable adverse effect on performance.

[boomfish]

I've decided not to create another patch. How about a nice long blog post instead?

[cfgrok]

Fixed in revision 280