Progress reporting functionality

[soxjke]

This implements progress reporting

[couchdeveloper]

Hi Petro

Thank you very much for the useful addition. I've couple of questions, though:

1. Why do we need the OSSpinLock _progressSpinLock?

   It seems to me, all accesses to the added ivars happen on the shared sync queue and should be synchronized anyway.

2. Why did you consider using pointers to the new C++ ivars _progressHandlers and _children?

   It's possible to use C++ non-POD types as ivars in Objective-C++ class. That is, having an ivar of type std::list, the c-tor and the d-tor will be called when appropriate. The only requirement is, that the type has a default c-tor.
   I understand it's a tradeoff: pointers may reduce the size of the promise and initialising a NULL pointer is for free. However it requires an additional new and delete for both when the containers are required, and later when deallocating the promise.

   An alternative approach would just use the container values directly. Initialising std containers is quite cheap, and the size is something about of three pointers.

3. What about a subclass of RXPromise, e.g. RXProgressPromise?

   What are the pros and cons?

4. The type of the progress value is a float. Why did you choose this, and not for example double or id?

And lastly:
Why is a progress state a property of a promise?

This is actually a quite opinionated and debatable question. IMHO, this would be a more appropriate property for a task - rather than a promise. A promise isn't a task, though - but there's also no abstraction for a task in the Promise library - so this is a dilemma ;)

Thanks again, I'm eager to here your comments and opinions :)

[couchdeveloper]

Here, progress will be accessed from different threads. We need to ensure the access is thread-safe, for example ensuring the execution context is set to the main thread, or use a dedicated serial dispatch queue. Note, the unit test itself will execute on the main thread, too.

This data race exists basically in all progress test.

It was interesting to experience the effect when running the tests: they intermittently failed one my iMac.

Note: I've made a new topic branch which fixes these issues in all tests. You may now apply these changes, or I merge the new fixed branch into master.

[soxjke]

Hi Andreas, thanks for the comprehensive feedback!

I'll evaluate it over the weekend and will come up with my thoughts / code

[soxjke]

Hi Andreas! The following are my thoughts on your feedback:

Why do we need the OSSpinLock _progressSpinLock?

It seems to me, all accesses to the added ivars happen on the shared sync queue and should be synchronized anyway.

Answer: I thought about concurrent std::list mutations, however, your point is completely correct, access is synchronized via Shared.sync_queue so i will get rid of unnecessary lock

Why did you consider using pointers to the new C++ ivars _progressHandlers and _children?

Answer: Your guess is correct, i thought exactly about the size of the resulting promise. However, two std::list sizes won't be that much, especially, if progress functionality will be extracted into subclass

What about a subclass of RXPromise, e.g. RXProgressPromise?

Answer: I see it's with following prospectives:

• Pros: More clear responsibility separation, more lightweight promises, that don't need progress
• Cons: More complicated implementation, since we will have to hook child promise emission from -registerWithExecutionContext:onSuccess:onFailure:onProgress:returnPromise: in a subclass, we'll have to check respondsToSelector: when chaining progress to children promises.

Personally I see no strong evidence in extracting progress functionality into subclass.

The type of the progress value is a float. Why did you choose this, and not for example double or id?

Answer: I'd like to emphasize on that the desired functionality is exactly the progress value. Why not double? - don't think the double precision is needed for fractional values of progress reporting. Float has 6-7 meaningful decimal signs precision, which i think is quite enough for this purpose. Why not id - i'd like to keep this as a progress values, having an id will transform RXPromise to stream of values over time - something like RACSignal from ReactiveCocoa

this would be a more appropriate property for a task - rather than a promise. A promise isn't a task, though - but there's also no abstraction for a task in the Promise library - so this is a dilemma

Answer: I had concern about whether adding progress functionality to RXPromise is appropriate before starting this code. I understand what kind of abstraction promise is, and i clearly understand that it isn't the task. However, looking at examples, i see that promises are mostly used in a bit simplified ways and scenarios:

• To avoid "callback hell". People use promise to transform API contract from accepting completion as an argument into returning some operation result abstraction. This "cleans up" ObjC and also inverts caller / callee dependencies. This also simplifies passing operation result over the caller code;
• As an connecting token. In current architecture i'm working on, promises are used as a token between client & service code. By "client" and "service" i mean parts of different application layers, this can be UI, network, DB or whatever else.

Keeping in mind above usages, i think we can have progress implemented on promise at all, since this will be very handy in terms of providing application layers interconnectivity functionality. Regarding the progress property - i think we can get rid of it at all, because it doesn't make sense in any particular moment of time since it's transient and meant to be delivered to child promises / clients by value through RXPromise synchronization mechanisms.

Summary

Looking forward to see your feedback and fixing mentioned above issues meanwhile

[soxjke]

@couchdeveloper feel free to review fixes / leave comments, appended pull-request with them. Also, feel free to delete soxjke-master branch, i've merged the changes.

[couchdeveloper]

I very much appreciate your comments and reasoning.

So, the OSSpinLock can be omitted and the C++ containers became values (this makes the code a bit more concise).

I do have a few improvements, though ;)

First, I need to explain a rather subtle and undocumented feature. In order to implement cancellation, I required each promise to remember their "returned promise" - which will be created in method registerWithExecutionContext:. So basically, a promise needs some container remembering their children as a weak pointer. I admit, it's not documented :( - so I try to explain that here.

It seems to me, this is exactly what you have accomplished with the member variable _children.

In my implementation, though, this container became a shared container of type std::multimap<Key, Value> - which is a static variable named assocs defined in file RXPromise+Private.h, where key is a pointer to the owning promise, and the value is a weak pointer to the "returned promise". That is, all promises share the same container instance.

I used this shared container approach in order to make the memory layout of a promise as small as possible, scarifying some other properties.

If I'm not wrong, assocs may be utilised for your purpose as well. It seems, you only store the "returned promise" into the _children container. This is exactly the purpose of assocs.

This is how it works:

Enqueueing a "returned promise":

Shared.assocs.emplace((__bridge void*)(blockSelf), weakReturnedPromise);

Dequeuing a child promise:

void const* key = (__bridge void const*)(self);
Shared.assocs.erase(key);

Iterating over the children:

void const* key = (__bridge void const*)(self);
auto range = Shared.assocs.equal_range(key);
while (range.first != range.second) {
    [(*(range.first)).second cancelWithReason:reason];
    ++range.first;
}

Note: Accesses MUST be executed on the shared sync queue!

So, if the Shared.assocs container can be used as a replacement for _children, we have yet another candidate which can be omitted :)

IFF _children can be replaced by Shared.assocs, it may look as follows:

void const* key = (__bridge void const*)(self);
auto range = Shared.assocs.equal_range(key);
while (range.first != range.second) {
    [(*(range.first)).second updateWithProgress:progress];
    ++range.first;
}

However, there's a caveat:

Currently, enqueueing the returned promise happens only when the handler has been executed. This is too late for the progress in order to be working correctly with the children. That is, the code above must be moved to a place where it is correct for both.

---

Other comments will be included in the diffs:

[couchdeveloper]

The enclosing if-statement if (_progressHandlers.size()) (not the block!) may be omitted. Just iterating over an empty list should work, too.

[soxjke]

Hi Andreas. The comments above make much sense. I'd verify Shared.assocs, seems i missed them - i thought returnedPromise is alive only in terms of it's generating handler execution context. I'll evaluate this possibility and simplify the code.

[couchdeveloper]

Ok, thank you too. ;)
The changes are actually quite minimal and it should be quite effortless to make the changes. I have a working version which passes all tests.

But we are not yet finished ;)

Now, I have a question, how forwarding the progress value to the children should work.
Consider the following scenario:

RXPromise* promiseTask1 = [task1 start];
RXPromise* promiseResult = promiseTask1.then(^id(Param* result){
    Task2* = [Task2 alloc] initWithParam: result];
    return [task2 start];
}, ^id(NSError*error){
    ...
});

Here, we have two tasks, task1 and task2 which will be called sequentially.

Suppose, task1 takes 100 WorkUnits and task2 takes 50 WorkUnits, where the "WorkUnits" values set the supposed duration of the tasks in relation to each other.

If the mechanic is simply passing the progress through to the child promise promiseResult - it would not correctly reflect the fact, that promiseResult has a continuation which returns another promise, which is supposed to take "significant" time to complete in relation to task1, and that only the returned promise of task2 will eventually complete the resulting promise promiseResult. That is, this would just report the progress of task 1 - and not consider the progress of task 2.

What I really want is, if I would observe the progress of the result promise, I would expect something like below:

progress
0.0                  0.67         1.0
|--------------------|------------|
^
Start Task1
                     ^End Task1 
                     ^Start Task2 
                                  ^End Task2 

If I think about it, in order to realise this form of progress, a promise would need to know its supposed value of "WorkUnit" from its "resolver". Since a promise will be created with then (for example), then would need an additional parameter "workUnit" in addition to the progress function.
Additionally, the root promise - which will be completed by the task, needs that value too.