readStream closing fix #204

Open
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants

bpung commented Jun 3, 2011

I ran into a consistently reproducible crasher when hitting the same url from two different reusable asihttprequest objects, one immediately after the other, where that url will redirect to another url and caching is set to ASIOnlyLoadIfNotCachedCachePolicy for both requests.

When rebuilding a reusable request to follow a redirect we need to make sure the old readStream is destroyed. There was previously no direct check on this. The old readStream is almost always done at this point, but the above example resulted in a case where the readStream for the original url of the second request object is not done at the time it gets redirected because its information is found in the cache after the first request object puts it there and we overwrite the second request objects original readStream while it is still returning data.

Detailed information is included in the commit log.

Brandon Pung added some commits Jun 3, 2011

When performRedirect is called we should destroy the old readStream b…
…efore rebuilding the request with the new stream in case the existing stream had not finished, which could be the case if the full data for the old stream was found in the cache after its first bits had returned but before it had finished. Here is a detailed example of a way this situation can occur:

* Consider you have two reusable requests R1 and R2 that both request the same URL
* Start these request one right after the other

(in chronological order of how events play out)

R1:
        * Starts with an initial readStream R1S1. The request is not in the download cache so it goes and starts pulling the stream
R2:
        * Starts with an initial readStream R2S1 (same url as R1S1). The request is not in the download cache so it goes and starts pulling the stream
R1:
        * The R1S1 stream finishes downloading its data (so no more data will be coming in on R1S1)
        * It calls handleStreamComplete and the data for the url gets stored in the cache
        * handleStreamComplete sees it is a redirect request
        * [self main] reuses R1 and sets up a new readStream which we'll call R1S2 with the redirected url
        * Redirected url is not in the download cache so it goes and starts pulling the stream
R2:
        * First bits of data return on R2S1 stream
        * handleBytesAvailable calls readResponseHeaders
        * readResponseHeaders check if request was previously cached and it WAS (since R1 cached it after R1S1 finished).
        * readResponseHeaders calls useDataFromCache which calls performRedirect which calls [self main]
        * [self main] reuses R2 and sets up a new readstream R2S2 with the redirected url
        * When the new readStream R2S2 is set it overwrites the old R2S1 stream but R2S1 WAS NOT FINISHED pulling data
        *** This is because so far it has only returned its first bytes of data and when it was found in the cache we moved on
        * The redirected url for R2S2 is not in the download cache so it goes and starts pulling the stream

* R1 and R2 finish their R1S2 and R2S2 streams which each pull the contents of the redirected url and both requests are closed
* The R2S1 stream returns with the rest of the content of its request but R2 has already been finished and dealloced
* Crash occurs in ReadStreamClientCallBack(CFReadStreamRef readStream, CFStreamEventType type, void *clientCallBackInfo) because clientCallBackInfo is deallocated.
Owner

pokeb commented Jun 4, 2011

This sounds like a similar problem to this one:

18a62f6

Are you using a recent version of ASIHTTPRequest that includes this fix?

Closing the readStream if it is removed from from the runloop would cause problems in other areas - bandwidth throttling and persistent connections rely on the stream staying open.

Best,

Ben

bpung commented Jun 7, 2011

Thanks for the reply Ben.

I'm using the latest (v1.8.1-8 at this time). The issue is similar to the one you linked to, in that they are both cases where the readStream property was being overwritten while the existing readStream was still in action, but the cause is different and the fix for that bug doesn't fix this one.

I'll look into this further and try to put together a basic test case that causes the crasher. I'll follow up in this thread.

bpung commented Jun 14, 2011

Here is a quick example of how you can make two requests that will throw this error. Let me know if you have any trouble reproducing this test case.

Create a view controller with two ASIHTTPRequests r1 and r2 that are retained, nonatomic properties. Then fire off those two requests from the view controller (on viewDidLoad or a button press action or anything) like this:

******************* In your view controller action *******************************

NSURL *urlThatWillRedirect = [NSURL URLWithString:@"http://www.allseeing-i.com/"];

[self setR1:[ASIHTTPRequest requestWithURL:urlThatWillRedirect]];
[self setR2:[ASIHTTPRequest requestWithURL:urlThatWillRedirect]];

[r1 setDefaultResponseEncoding: NSUTF8StringEncoding];
[r1 setDelegate: self];
[r1 setTimeOutSeconds: 20];
[r1 setDownloadCache:[ASIDownloadCache sharedCache]];
[r1 setCachePolicy: ASIOnlyLoadIfNotCachedCachePolicy];
[r1 setCacheStoragePolicy: ASICachePermanentlyCacheStoragePolicy];
[r1 setSecondsToCache: 60 * 60 * 24];

[r2 setDefaultResponseEncoding: NSUTF8StringEncoding];
[r2 setDelegate: self];
[r2 setTimeOutSeconds: 20];
[r2 setDownloadCache:[ASIDownloadCache sharedCache]];
[r2 setCachePolicy: ASIOnlyLoadIfNotCachedCachePolicy];
[r2 setCacheStoragePolicy: ASICachePermanentlyCacheStoragePolicy];
[r2 setSecondsToCache: 60 * 60 * 24];

[r1 startAsynchronous];
[r2 startAsynchronous];

Then add the following request finished implementation to remove the requests when they're done:


- (void)requestFinished:(ASIHTTPRequest *)req {
if (req == r1) {
[self setR1:nil];
}
else if (req == r2) {
[self setR2:nil];
}
}


Hit your view controller action and after several seconds it will error out with an EXC_BAD_ACCESS in ReadStreamClientCallBack due to the situation I described above. Notice I'm using "http://www.allseeing-i.com/" as the initial url, which will cause a redirect to "http://allseeing-i.com/".

fassko commented Nov 5, 2011

Thanks, it worked for me! :)

Was this ever solved? I am running into this same problem.

I am still running into this problem. Is this a fix if I am not using bandwidth throttling?

This seems to have fixed a crash that I was having. My app relies on caching and redirects in download queues that can sometimes be created and canceled rapidly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment