Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 347974

Summary:

[WireAdmin] Fails OSGi CT tests: testValueFilteringDeltaAbsolute and testValueFilteringDeltaRelative

Product:

[Eclipse Project] Equinox

Reporter:

BJ Hargrave <hargrave>

Component:

Compendium

Assignee:

BJ Hargrave <hargrave>

Status:

RESOLVED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

jwross, s.boshev, tjwatson

Version:

3.7

Flags:

tjwatson: review+
jwross: review+
s.boshev: review+

Target Milestone:

3.7 RC4

Hardware:

All

OS:

All

Whiteboard:

Attachments:

Description	Flags
Fix to prime the pump for previous values	none
[updated] Fix to prime the pump for previous value	none
[updated2] Fix to prime the pump for previous value	s.boshev: review+

Description BJ Hargrave

2011-06-01 12:06:50 EDT

WireAdmin spec allows flow control over the wire: Section 108.7. There are several filters which depend upon the "previous" value which seems to be defined by WIREVALUE_PREVIOUS as the previous data value reported to the Consumer. That is, the Consumer actually received the value. Most of the filter types are based upon the previous reported value. So when the first produced value is updated onto the wire, there has not been a previous value, so none of those filter types will report the value to the consumer and we are now caught in a loop of never having a previous value and never delivering a value.

The CT assumes that an implementation will "prime the pump" by considering the first undelivered value as a previous value for purposes of evaluating the filter for the next value.

The OSGi CT methods, testValueFilteringDeltaAbsolute and testValueFilteringDeltaRelative, both fail because no values are ever delivered to the test Consumer.

Comment 1 Thomas Watson

2011-06-01 12:08:58 EDT

It would be good to get this in RC4 so Equinox can be used as the RI for WireAdmin.  BJ, please attach the patch.

Comment 2 BJ Hargrave

2011-06-01 12:17:43 EDT

Created attachment 197113 [details]
Fix to prime the pump for previous values

The patch "primes the pump" by setting the first value passed to the wire as the previous value even if that value is not actually passed to the consumer.

The patch also makes the proper distinction between lastValue, which is the last value provided by the Producer, and previousValue, which is the previous value supplied to the Consumer. When wire flow control is in effect, these 2 values are often different since some values are not actually supplied to the consumer.

Finally the patch adds finally block to properly set lastValue and/or previousValue due to early return from the method.

Comment 3 Thomas Watson

2011-06-01 12:26:22 EDT

John and Stoyan, please review.

Comment 4 Thomas Watson

2011-06-01 12:36:56 EDT

Initial review of the patch looks good, but I will admit that I am confused by the field names "lastValue" and "previousValue".  perhaps "lastFromProducer" and "lastToConsumer".

Need to up date copyright date.

Comment 5 BJ Hargrave

2011-06-01 12:58:33 EDT

(In reply to comment #4)
> Initial review of the patch looks good, but I will admit that I am confused by
> the field names "lastValue" and "previousValue".  perhaps "lastFromProducer"
> and "lastToConsumer".
> 

The names match terms used on the spec. I felt it was important to use matching names.

> Need to up date copyright date.

Do I just change 2008 to 2011? Or are more changes needed?

Comment 6 Thomas Watson

2011-06-01 13:45:48 EDT

(In reply to comment #5)
> (In reply to comment #4)
> > Initial review of the patch looks good, but I will admit that I am confused by
> > the field names "lastValue" and "previousValue".  perhaps "lastFromProducer"
> > and "lastToConsumer".
> > 
> 
> The names match terms used on the spec. I felt it was important to use matching
> names.

Fair enough, I was not reading the spec when I reviewed.

> 
> > Need to up date copyright date.
> 
> Do I just change 2008 to 2011? Or are more changes needed?

Yes, that will do.  You could add a line under Contributors:

   IBM Corporation - bug fix 347974

Comment 7 BJ Hargrave

2011-06-01 14:02:28 EDT

Created attachment 197125 [details]
[updated] Fix to prime the pump for previous value

Updated patch with modified copyright notice.

Comment 8 Thomas Watson

2011-06-01 14:05:51 EDT

Looks good.

Comment 9 Stoyan Boshev

2011-06-01 15:04:00 EDT

The patch is OK and fixes the original bug. But it seems to me it introduces a new bug.
It is calling parent.notifyListeners(this, WireAdminEvent.WIRE_TRACE, null) regardless the consumer's updated method might not have been called.
This method must be called only if the consumer's updated method is called.

Comment 10 Stoyan Boshev

2011-06-01 15:15:10 EDT

Created attachment 197150 [details]
[updated2] Fix to prime the pump for previous value

This is a small update of the patch. It fixes the only issue found in the previous patch

Comment 11 BJ Hargrave

2011-06-01 15:24:16 EDT

(In reply to comment #9)
> The patch is OK and fixes the original bug. But it seems to me it introduces a
> new bug.
> It is calling parent.notifyListeners(this, WireAdminEvent.WIRE_TRACE, null)
> regardless the consumer's updated method might not have been called.
> This method must be called only if the consumer's updated method is called.

It is supposed to trace even if the consumer is not called. From the spec 108.11:

WIRE_TRACE - The Producer service has called the Wire.update(Object) method with a new value or the Producer service has returned from the Producer.polled(Wire) method.

I had fixed this bug in my patch but had forgotten to mention it in my patch description.

Comment 12 Thomas Watson

2011-06-01 15:38:06 EDT

BJ, Stoyan and I discussed this and believe we should go with Stoyan's patch.  It appears the javadoc for wireadmin is in conflict.  The current RI behaves as Stoyan's patch and we should base on that for the decision.  We will get the spec clarified to be consistent in the next spec release.

Comment 13 BJ Hargrave

2011-06-01 15:50:08 EDT

Fix released to HEAD for 3.7 RC4.