RKRequest Timeout #491

Closed
wants to merge 11 commits into
from

Projects

None yet

3 participants

@bmorton

I've implemented the patch for RKRequest timeouts that was outlined on the Google group by Blake and I'd like to get this pull request started to make sure I'm on the right track with it.

I'm a Ruby developer that has been dabbling in iOS development for the past couple years and finally had the opportunity to build an application and have loved everything about RestKit. It seems others involved in the RestKit project appreciate Ruby, too :)

The testing side of me is dying to get some tests implemented for this, but I'm at a bit of a loss as to where to start. I've got the test suite running and everything passes (before and after my patch ;). I have a couple ideas of how I might tackle it, but I'd love a shove in the right direction.

bmorton added some commits Jan 5, 2012
@bmorton bmorton Add RKRequestConnectionTimeoutError to throw when our connection time…
…out has been exceeded.
d864c57
@bmorton bmorton Add support for timing out an RKRequest via a timeoutTimer and a time…
…out property.

* Add timeoutInterval property on RKRequest with a default value of 120.0
* Add timeout method that is called by the timer when the timeout interval has been exceeded to cancel the request and return an error via didFailLoadWithError:
* Add invalidateTimeoutTimer method that is called by RKResponse when the NSURLConnection begins receiving data.
* Add call to invalidateTimeoutTimer in RKRequest cancelAndInformDelegate: so we don't have a dangling timer.
* Add timer creation to sendAsynchronously and sendSynchronously
a3465ad
@bmorton bmorton Invalidate the RKRequest timer in RKResponse when the NSURLConnection…
… begins receiving data.
bcd5762
@bmorton bmorton Add timeoutInterval property to RKClient so that it can be passed on …
…to RKRequest.
636dfff
@bmorton bmorton Responsibly invalidate the timeoutTimer when the RKRequest is dealloc…
…ated. Move location of timer creation for asynchronous request.
c934cd2
@bmorton bmorton Move the invalidateTimeoutTimer call in RKResponse from didReceiveDat…
…a to didReceiveResponse and didFailWithError so we are sure its always called and as early as possible.
0ae860c
@blakewatters
The RestKit Project member

Thanks, this looks good. I'll drill in and hopefully merge this evening.

@roland9

just encountered this issue in a project I'm working on - so if you merge would be perfect timing :)

@blakewatters
The RestKit Project member

This patch looks good but we should get some unit testing in place. This should be fairly easy to test.

If you look in the Specs/ directory, there are a bunch of examples for how to write tests and the Docs/TESTING.md document gives a quick overview of the environment.

Most of the request based unit testing works off of a class called RKSpecResponseLoader. Basically what it does is serves as a delegate for an RKRequest/RKObjectLoader that is being tested. The internals of that class set up and utilize an NSTimer and spins the run loop while a request is in progress. This has the effect of making an asynchronous operation feel synchronous from the perspective of the unit test (pseduocode follows):

RKRequest *request = [RKRequest requestWithURL:URL];
request.delegate = specResponseLoader;
[request sendAsynchronously];
[specResponseLoader waitForResponse];

Based on the work that you've done here we may be able to shift that timer responsibility out of RKSpecResponseLoader and into the RKRequest and use your timer instead. To unit test it, we'd add the didTimeout delegate method to the RKSpecResponseLoader's implementation and then use OCMock to create a partial mock of the spec response loader instance and setup an expectation that the did timeout method was invoked. We'll also likely need to add a method to the Sinatra server that sleeps for X seconds where X is greater than the timeout we've configured on the request. Then we just fire a request to the sleep action, let it timeout, and verify the invocation on the OCMock instance.

Does that give you enough context to try and tackle the unit testing? (p.s. you'll likely need to use andForwardToRealObject on the OCMock expectation so the response loader knows about the timeout event)

@bmorton

Sweet, that should give me enough to get something in place. I'll take a look at it tonight when I get home and hopefully get this thing nailed down.

Thanks!

bmorton added some commits Jan 10, 2012
@bmorton bmorton Merge branch 'master' into timeouts d8fed85
@bmorton bmorton Add /timeout route to fake a long response for testing RKRequest's ti…
…meout interval. We need to leave this around 4 seconds so we don't hold up the ruby process too long and cause the tests launched after to fail.
d5bd2a4
@bmorton bmorton Add test for making sure that an RKRequest fired with a timeout set w…
…ill return the proper error if the remote server takes too long to start responding.
41d4776
@bmorton

I've implemented a response in the sinatra server and a test in RKRequestSpec for testing that timeouts are working as expected. I was able to mock the RKSpecResponseLoader and have it expect the error code that is supposed to be thrown.

I've set the timeout to be 3 seconds and I have the sinatra response sleeping for 4 seconds. This shouldn't hold up the test suite too long while still giving this test enough time.

I do see what you mean with the RKSpecResponseLoader timer and I'll probably dive into there after this gets merged in and see if that can get cleaned up a bit.

Thanks again for the push in the right direction.

@bmorton

This is good to go now, as well. Did you see any other issues with it?

@blakewatters
The RestKit Project member

Brian -

Sorry for the delay in getting this merged. I just merged it to master in 55c72a5

Everything looks great except for one minor detail. Please avoid merging master into your feature branches. You should rebase against master to get your changes current. I prefer to have only see merge commits that have meaning and ultimately master should be a destination for merges, not a source of them. I cut a local feature branch and replayed your changes to eliminate the upstream merge. I am going to throw together a contribution document today on the wiki detailing this stuff -- no worries on this branch since you can't know things I haven't told you :-)

Thanks for another solid contribution!

Cheers,
Blake

@fishman fishman added a commit to fishman/RestKit that referenced this pull request Dec 4, 2013
@fishman fishman add timeoutinterval from old pull request RestKit#491 327f052
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment