Mark ASRunLoopQueue as drained if it contains only NULLs

[cesteban]

I wrote a failing tests for #518. Then made it pass.

Apparently, the problem is NSPointerArray won't be marked as needsCompaction when stored weak pointers become NULL. This makes subsequent calls to compact to return early without effectively removing NULL's. As a result ASRunLoopQueue is never marked as drained, event though there is no more work to do.

I added an isEmpty public property to ASRunLoopQueue which is only used in tests. That's not pretty, but I went this way because IMO the property makes sense in an API like that (if you have a queue, it's very natural to have a way to know if it's empty). If you don't want to have this property, we need to find out another way for the test to sense that the queue has been marked as drained.

Finally, I believe ASRunLoopQueue should have a lot more tests, given the importance of the component. I would try to find the time to write a bunch of them, but that should be a different branch IMO.

Please, let me know your thoughts.

Edit (huy): Closes #518.

[cesteban]

I finally covered ASRunLoopQueue with tests.

[nguyenhuy]

First of all, thanks for investigating the problem and submit this patch! Extra kudos for the new unit tests!

That being said, I have 1 small problem with the new _isQueueDrained state, that is we need to do extra work to make sure it's up-to-date. If the problem lies within NSPointerArray (and there is indeed a radar for it), maybe we should somehow force it to do the right thing? I think there are a couple of things we can do:

• Use allObjects.count instead of count. Unlike the latter, the former API shouldn't include NULL pointers.
• Whenever needed, force the pointer array to compact by inserting a nil pointer before calling compact, as suggested here.

Let me know what you think!

[nguyenhuy]

Nit: Please add the PR's URL.

[cesteban]

Yup, sorry!

[nguyenhuy]

Also, please update the license in ASRunLoopQueueTests.m as suggested by Danger above.

[cesteban]

@nguyenhuy Actually, my first implementation that passed the test was something similar to this:

    NSInteger foundItemCount = 0;
    for (NSInteger i = 0; i < internalQueueCount && foundItemCount < maxCountToProcess; i++) {
      //...
    }

    if (foundItemCount == 0) {
      // If _internalQueue holds weak references, and all of them just become nil, then the array
      // is never marked as needsCompletion, and compact will return early, not removing the nils.
      // Inserting a nil here ensures the compaction will take place.
      [_internalQueue addPointer:nil];
    }

    [_internalQueue compact];
    if (_internalQueue.count == 0) {
      isQueueDrained = YES;
    }

Do you prefer this kind of approach? I can recover it, just let me know.

[nguyenhuy]

@cesteban Yes, I think it's a cleaner approach. May worth adding the StackOverflow and OpenRadar links.

[cesteban]

Thanks @nguyenhuy. There we go!

[nguyenhuy]

Great win! Thank you!