delegate getting called after being remove

[Joseph Heenan]

Hi all,

I'm seeing odd behaviour when using async requests, with my delegate
getting called after I've done [request setDelegate:nil];

When this happens, I can see in the request that's passed to the
delegate that the delegate field is set to nil.

I think it could be a race condition, in that this code:

// Let the delegate know we are done
if ([self didFinishSelector] && [[self delegate] respondsToSelector:
[self didFinishSelector]]) {
[[self delegate] performSelectorOnMainThread:[self
didFinishSelector] withObject:self waitUntilDone:[NSThread
isMainThread]];
}

could be inside the 'performSelectorOnMainThread' call when the main
thread sets the delegate to nil.

I think the solution could be to add a wrapper something like:

- (void) callFinishSelector
{
if ([self didFinishSelector] && [[self delegate] respondsToSelector:
[self didFinishSelector]]) {
[[self delegate] performSelector:[self didFinishSelector]
withObject:self];
}

and change the above code to:
// Let the delegate know we are done
if ([self didFinishSelector] && [[self delegate] respondsToSelector:
[self didFinishSelector]]) {
[[self delegate] performSelectorOnMainThread:[self
callFinishSelector] withObject:self waitUntilDone:[NSThread
isMainThread]];
}

(completely untested code!)

Just wondered if anyone has run into this or anything similar before
and whether I'm heading in the right direction or not.


Joseph

[Ben Copsey]

Hi Joseph

Ok, I think I see what you mean.

Logically, there's a risk in the current implementation that a delegate can be called after the request's delegate property has been set to nil, though I would imagine the likelihood of this happening is small. I think it would only happen if the call to the delegate has already been scheduled on the main thread, but hasn't yet run, when you set the delegate to nil (the 'cancelledLock' lock should prevent the call to the delegate being performed at all otherwise).

It sounds like your solution would work. The main issue is that ASIHTTPRequests have 4 separate delegates (delegate, queue, uploadProgressDelegate, downloadProgressDelegate) and lots of different methods for each. I'm wondering if there's a more generic solution - adding extra methods for all the main thread calls would add quite a lot of code... :)

Best

Ben

[Joseph Heenan]

Hi Ben,

Thanks for your thoughts on this!

On Jul 1, 1:38 pm, Ben Copsey <b...@allseeing-i.com> wrote:
> Logically, there's a risk in the current implementation that a delegate can be called after the request's delegate property has been set to nil, though I would imagine the likelihood of this happening is small. I think it would only happen if the call to the delegate has already been scheduled on the main thread, but hasn't yet run, when you set the delegate to nil (the 'cancelledLock' lock should prevent the call to the delegate being performed at all otherwise).

Yes, that's right. My application is a little unusual, in that it
makes http requests in response to control messages from a server, so
can end up making/destroying several requests quickly within a second
or two - I've seen the problem happen about a dozen times in 2 days of
development, but it is a little difficult to reproduce reliably.

> It sounds like your solution would work. The main issue is that ASIHTTPRequests have 4 separate delegates (delegate, queue, uploadProgressDelegate, downloadProgressDelegate) and lots of different methods for each. I'm wondering if there's a more generic solution - adding extra methods for all the main thread calls would add quite a lot of code... :)

Yes, I came to the same conclusion :-)

I did ask on stackoverflow (
http://stackoverflow.com/questions/3158356/how-to-handle-setdelegate-when-using-multipe-threads
) and got one other suggestion, which is to retain the delegate for
the duration of the performSelectorOnMainThread.

Whilst that doesn't 100% solve the issue (the delegate could still be
called after having removed itself) as far as I can see it does 100%
prevent the delegate being called after it's being destroyed, which
may well be all that really matters.

Thanks

Joseph

[Ben Copsey]

Hi Joseph

I think the problem of the delegate being called after it is set to nil is something I should sort out, even if it ends up being lots more code. I can imagine there are plenty of potential problems otherwise, even if the risk of this happening is small in most cases. I'll add it to the list.

Many thanks for pointing out this issue!

Best

Ben

[Joseph Heenan]

Hi Ben,

Ben Copsey wrote:
>>> Logically, there's a risk in the current implementation that a delegate can be called after the request's delegate property has been set to nil, though I would imagine the likelihood of this happening is small. I think it would only happen if the call to the delegate has already been scheduled on the main thread, but hasn't yet run, when you set the delegate to nil (the 'cancelledLock' lock should prevent the call to the delegate being performed at all otherwise).
>> Yes, that's right. My application is a little unusual, in that it
>> makes http requests in response to control messages from a server, so
>> can end up making/destroying several requests quickly within a second
>> or two - I've seen the problem happen about a dozen times in 2 days of
>> development, but it is a little difficult to reproduce reliably.

> I think the problem of the delegate being called after it is set to nil is something I should sort out, even if it ends up being lots more code. I can imagine there are plenty of potential problems otherwise, even if the risk of this happening is small in most cases. I'll add it to the list.

I've had a go at fixing this - I think it ended up not being too much
more code after having a think through ways to do it, and should
guarantee the delegate(s) are never called after being removed.

Change is here:

http://github.com/jogu/asi-http-request/commit/6d4c6c9dd7d5dc6cd8b9cf4c372d7ce45016083b

It doesn't cover the authentication delegate, as it looks harder and
I've already spent a bit too much time on this.

(I eventually came to the conclusion the other suggestions I'd got on
stackoverflow, like retaining the delegate during the calls, were in
fact wrong and caused other crashes/race conditions! :-( )

If you've any suggestions to improve the patch let me know and I'll try
and fix them up.

Joseph

[Ben Copsey]

Hi Joseph

>>>> Logically, there's a risk in the current implementation that a delegate can be called after the request's delegate property has been set to nil, though I would imagine the likelihood of this happening is small. I think it would only happen if the call to the delegate has already been scheduled on the main thread, but hasn't yet run, when you set the delegate to nil (the 'cancelledLock' lock should prevent the call to the delegate being performed at all otherwise).
>>> Yes, that's right. My application is a little unusual, in that it
>>> makes http requests in response to control messages from a server, so
>>> can end up making/destroying several requests quickly within a second
>>> or two - I've seen the problem happen about a dozen times in 2 days of
>>> development, but it is a little difficult to reproduce reliably.
>> I think the problem of the delegate being called after it is set to nil is something I should sort out, even if it ends up being lots more code. I can imagine there are plenty of potential problems otherwise, even if the risk of this happening is small in most cases. I'll add it to the list.
>
> I've had a go at fixing this - I think it ended up not being too much more code after having a think through ways to do it, and should guarantee the delegate(s) are never called after being removed.
>
> Change is here:
>
> http://github.com/jogu/asi-http-request/commit/6d4c6c9dd7d5dc6cd8b9cf4c372d7ce45016083b

Thanks very much for this! I'll take a proper look over the weekend when I have a bit more time.

Thanks again,

Ben

[Joseph Heenan]

Hi Ben,

On Jul 14, 10:47 am, Ben Copsey <b...@allseeing-i.com> wrote:

> > I've had a go at fixing this - I think it ended up not being too much more code after having a think through ways to do it, and should guarantee the delegate(s) are never called after being removed.
>
> > Change is here:
>

> >http://github.com/jogu/asi-http-request/commit/6d4c6c9dd7d5dc6cd8b9cf...

>
> Thanks very much for this! I'll take a proper look over the weekend when I have a bit more time.

No problem. Let me know if you spot any problems or have any
questions.

I did find one memory leak whilst doing further stress testing of my
app under instruments, there's a fix for that here:

http://github.com/jogu/asi-http-request/commit/ddf967631d31e9e0b18186faff14b2e0188827a0

(There are a couple of other fixes in that tree too - I'm not sure
I've seen any problems occur without those other fixes, but equally I
believe they fix problems that could occur.)

Cheers,

Joseph