Fixed retain cycle in ISpdyTimer

[cristiankocza-sv]

This reduces the number of leaks related to dispatch objects.

[cristiankocza-sv]

@indutny, @georgekola please review

[indutny]

The question is, how could it be leaking if the handler is always executed, or [timer clear] is called instead? I haven't seen any dispatch_object leaks after removing that define...

[cristiankocza-sv]

I profiled the app with and without the changes in this PR, and with the original code Instruments showed some dispatch-related leaks, and with the changes it did not. The "Call Tree" section of the Leaks instruments lead me to this method, this is why I added the weak reference, and retested the app.

Looked at dispatch_source_set_event_handler_f documentation, which says that the handler cannot be NULL, so it might be that the problematic block doesn't get removed (deallocated), thus keeping alive the ISpdyTimer object, and preventing its dealloc code to be called. So that logic might need to be improved too.

[indutny]

@cristiankocza I'm not sure which documentation you are referring here:

To unset the event handler, call dispatch_source_set_event_handler_f() and pass NULL as function.  This unsets the event handler regardless of whether the handler was a function
     pointer or a block. Registration and cancellation handlers (see below) may be unset in the same way, but as noted below, a cancellation handler may be required.

May I ask you to post a complete stack trace of the leak?

[cristiankocza-sv]

From the doxygen documentation of the dispatch_source_set_event_handler_f function:

/*!
 * @function dispatch_source_set_event_handler_f
 *
 * @abstract
 * Sets the event handler function for the given dispatch source.
 *
 * @param source
 * The dispatch source to modify.
 * The result of passing NULL in this parameter is undefined.
 *
 * @param handler
 * The event handler function to submit to the source's target queue.
 * The context parameter passed to the event handler function is the current
 * context of the dispatch source at the time the handler call is made.
 * The result of passing NULL in this parameter is undefined.
 */
__OSX_AVAILABLE_STARTING(__MAC_10_6,__IPHONE_4_0)
DISPATCH_EXPORT DISPATCH_NONNULL1 DISPATCH_NOTHROW
void
dispatch_source_set_event_handler_f(dispatch_source_t source,
    dispatch_function_t handler);

[indutny]

This seems to contradict the man page... Gosh, Apple!

What do you think about using dispatch_source_set_event_handler(source, ^() {}) without that weak reference thing? Will it fix the leak?

[cristiankocza-sv]

That might work, however I'd also recommend the weak approach, to follow the recommended practices when working with blocks,

[indutny]

Ok, let's do both.

[georgekola]

@indutny can you please check if the timer tests run fine with this change. We are finding leaking on iOS 8.4.x and hence want it resolved.

[indutny]

Appears to be working fine:

2015-09-29 12:48:09.534 test-runner[17227:7947505] + 'ISpdy server, using timers, should call single timer callback' [PASSED]
2015-09-29 12:48:11.794 test-runner[17227:7947505] + 'ISpdy server, using timers, should call recursive timer callback' [PASSED]
2015-09-29 12:48:12.926 test-runner[17227:7947505] + 'ISpdy server, using timers, should call two timers in a row' [PASSED]
2015-09-29 12:48:14.068 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v2 - no comp, should return body that was sent' [PASSED]
2015-09-29 12:48:15.203 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v2 - no comp, should return big body that was sent' [PASSED]
2015-09-29 12:48:15.816 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v2 - no comp, should timeout on slow responses' [PASSED]
2015-09-29 12:48:15.918 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v2 - no comp, should handle failures' [PASSED]
2015-09-29 12:48:17.040 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v2 - deflate, should return body that was sent' [PASSED]
2015-09-29 12:48:18.174 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v2 - deflate, should return big body that was sent' [PASSED]
2015-09-29 12:48:18.784 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v2 - deflate, should timeout on slow responses' [PASSED]
2015-09-29 12:48:18.891 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v2 - deflate, should handle failures' [PASSED]
2015-09-29 12:48:20.022 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v2 - gzip, should return body that was sent' [PASSED]
2015-09-29 12:48:21.156 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v2 - gzip, should return big body that was sent' [PASSED]
2015-09-29 12:48:21.670 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v2 - gzip, should timeout on slow responses' [PASSED]
2015-09-29 12:48:21.772 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v2 - gzip, should handle failures' [PASSED]
2015-09-29 12:48:22.915 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v3 - no comp, should return body that was sent' [PASSED]
2015-09-29 12:48:24.056 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v3 - no comp, should return big body that was sent' [PASSED]
2015-09-29 12:48:24.579 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v3 - no comp, should timeout on slow responses' [PASSED]
2015-09-29 12:48:24.686 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v3 - no comp, should handle failures' [PASSED]
2015-09-29 12:48:25.819 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v3 - deflate, should return body that was sent' [PASSED]
2015-09-29 12:48:26.957 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v3 - deflate, should return big body that was sent' [PASSED]
2015-09-29 12:48:27.567 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v3 - deflate, should timeout on slow responses' [PASSED]
2015-09-29 12:48:27.668 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v3 - deflate, should handle failures' [PASSED]
2015-09-29 12:48:28.796 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v3 - gzip, should return body that was sent' [PASSED]
2015-09-29 12:48:29.929 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v3 - gzip, should return big body that was sent' [PASSED]
2015-09-29 12:48:30.441 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v3 - gzip, should timeout on slow responses' [PASSED]
2015-09-29 12:48:30.544 test-runner[17227:7947505] + 'ISpdy server, sending requests to echo server, spdy-v3 - gzip, should handle failures' [PASSED]

[georgekola]

Ok, pasting the source code of libdispatch-442.1.4 (latest Apple has made available so far), calling with a handler of NULL correctly clears the handler as the man page says -- the header file has apparently not been updated

cc @cristiankocza @indutny @kostiadombrovsky @dhruvilv

void
dispatch_source_set_event_handler_f(dispatch_source_t ds,
dispatch_function_t handler)
{
dispatch_continuation_t dc;
dc = _dispatch_source_handler_alloc(ds, handler, DS_EVENT_HANDLER, false);
_dispatch_barrier_trysync_f((dispatch_queue_t)ds, dc,
_dispatch_source_set_handler);
}

DISPATCH_ALWAYS_INLINE
static inline dispatch_continuation_t
_dispatch_source_handler_alloc(dispatch_source_t ds, void *handler, long kind,
bool block)
{
dispatch_continuation_t dc = _dispatch_continuation_alloc();
if (handler) {
dc->do_vtable = (void *)((block ? DISPATCH_OBJ_BLOCK_RELEASE_BIT :
DISPATCH_OBJ_CTXT_FETCH_BIT) | (kind != DS_EVENT_HANDLER ?
DISPATCH_OBJ_ASYNC_BIT : 0l));
dc->dc_priority = 0;
dc->dc_voucher = NULL;
if (block) {
#ifdef BLOCKS
if (slowpath(_dispatch_block_has_private_data(handler))) {
// sources don't propagate priority by default
dispatch_block_flags_t flags = DISPATCH_BLOCK_NO_QOS_CLASS;
flags |= _dispatch_block_get_flags(handler);
_dispatch_continuation_priority_set(dc,
_dispatch_block_get_priority(handler), flags);
}
if (kind != DS_EVENT_HANDLER) {
dc->dc_func = _dispatch_call_block_and_release;
} else {
dc->dc_func = _dispatch_Block_invoke(handler);
}
dc->dc_ctxt = _dispatch_Block_copy(handler);
#endif /* BLOCKS _/
} else {
dc->dc_func = handler;
dc->dc_ctxt = ds->do_ctxt;
}
dispatch_trace_continuation_push((dispatch_queue_t)ds, dc);
} else {
dc->dc_func = NULL;
}
dc->dc_data = (void)kind;
return dc;
}