Implement NSURLProtocol.stopLoading() for delayed requests #96

leviathan · 2016-08-10T12:28:42Z

General description

Implemented the NSURLProtocol#stopLoading() logic in KakapoServer

After the latency feature has been added to Router, which allows to delay the request callback reporting, it is now possible that a request may be waiting for its callback, but KakapoServer gets commanded to cancel the particular request.

This pull requests makes the full roundtrip and adds the capability to "not complete" the request, if KakapoServer command so.

Primarily affected components

KakapoServer & Router

Depending components

none

Expected possible Pitfalls

none

Device and OS test coverage

Test on iPhone 5, iOS 8: untested
Test on iPhone 6S, iOS 9: leviathan
Test on iPad, iOS 8: untested
Test on iPad, iOS 9: untested
Test on Watch, watchOS 2: untested

Closes #88

codecov-io · 2016-08-10T12:34:50Z

Current coverage is 99.16% (diff: 100%)

Merging #96 into master will increase coverage by 0.01%

@@             master        #96   diff @@
==========================================
  Files            10         10          
  Lines           470        477     +7   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            466        473     +7   
  Misses            4          4          
  Partials          0          0

Powered by Codecov. Last update d8ee24c...e6ea7c8

joanromano · 2016-08-10T12:41:52Z

Source/Router.swift

@@ -101,6 +102,7 @@ public final class Router {
    }

    private var routes: [String : Route] = [:]
+    private var canceledRequests: [NSURLProtocol] = []


Careful, this would create a retain cycle between Server and Router objects. Not sure if we need a different approach here rather than storing references to Server (NSURLProtocol).

yeah, right. that's bad ... I'll find a solution

I solved the retain cycle by remembering the NSURL, which should be canceled.
fixed in c10b110

joanromano · 2016-08-10T12:49:33Z

Check code cov, merging this into master would decrease our test coverage a bit: please make sure to add unit tests that cover this new scenario, you can check RouterTests where we gather all unit tests for the Router.

joanromano · 2016-08-10T12:53:15Z

.gitignore

+# AppCode
+.idea
+.idea/
+


What is this for? Some auto generated files from AppCode?

Yes, these are meta files generated by AppCode (don't want to have them in the repo).

…S) logic better understandable

…sts for 'stopLoading'

leviathan · 2016-08-11T09:55:44Z

Ok, tasks for me

increase code coverage (see: https://codecov.io/gh/devlucky/Kakapo/compare/33379436a04123a97f34584ada21621dad8e5d3f...9c98c60c3b65fb6609b0f09efc7a9c297c7034d7?src=pr)
fix unit tests for RouterTests (see: https://travis-ci.org/devlucky/Kakapo/jobs/151453485)

@joanromano how do you run the unit tests locally?

MP0w · 2016-08-11T10:06:23Z

CMD+U should be enough, not working for you?

…uter (avoid accessing private vars)

leviathan · 2016-08-11T10:32:26Z

@MP0w codecov requests that I unit test private methods.
https://codecov.io/gh/devlucky/Kakapo/compare/33379436a04123a97f34584ada21621dad8e5d3f...f5d61a93890875a5cab3762afcc70d23786bb1d6

That's totally nonsense ... what shall I do?

I would need to access private var canceledRequests: [NSURL] = [], which is a private property on Router, which I cannot (will result in compiler error). And its private for a reason.

MP0w · 2016-08-11T10:38:27Z

@leviathan we are doing BDD so we want to test the behaviors. You don't need to test that method directly but tests the behavior of canceling a request. While testing the cancel request the lines of private method will be implicitly covered since the private methods are used when you cancel a request.
So an example could be:
Describe Cancelling requests
Context when intercepted by KakapoServer
It should not call the route handler
It should return an error
.....

You will just have to create a request and cancelling it before it starts and the private methods involved should be then covered . Let me know if you have any question.

joanromano · 2016-08-11T12:05:25Z

Tests/RouterTests.swift

+        else {
+            client.URLProtocol(self, didReceiveResponse: NSHTTPURLResponse(URL: requestURL, statusCode: 200, HTTPVersion: "HTTP/1.1", headerFields: nil)!, cacheStoragePolicy: .AllowedInMemoryOnly)
+            client.URLProtocolDidFinishLoading(self)
+        }
    }


This is a test NSURLProtocol just for the unit test target in order to intercept any outgoing calls that are not registered with Routers (some requests to http://www.test.com we test against). You shouldn't need to touch the RouterTestServer at all unless I am missing something.

…ter's request loading gets cancelled.

leviathan · 2016-08-12T08:06:12Z

@joanromano & @MP0w any remarks?
looks like the CI is satisfied 👍

MP0w · 2016-08-12T08:24:41Z

@leviathan I'm going in holidays for 1 week so I won't have time to help but could you guys make sure the tests are failing when the implementation is removed (not cancelling requests)?
For example should not start a request when request gets cancelled is never failing because the requestURL is initialized with nil and you expect.toEventually(beNil()).

For the implementation, I would really prefer a simple flag in the Server created by the system which is (IMO) more clean than keeping track of called requests. For example you could spawn 2 equal request at the same time with same URL (no-sense but to highlight the fragility) and the router would not know which one has to be cancelled.

I quickly talked with @joanromano and he known my opinion and his own, he will take care of the review since I won't be available.

Little side note: next time let's create separate PR for typos (ty very much) so it's easier to review, that's our fault since we didn't create any contributing guideline (will take care when I'm back).
Thank you very much for the efforts and for contributing, we really appreciate it!

Have a good weekend guys! 🎉 :micdrop:

joanromano · 2016-08-12T09:06:51Z

Source/KakapoServer.swift

+     The overall list of `Router` objects, which is know to all `KakapoServer` instances.
+     Visibility is on the `KakapoServer` class level, because `NSURLProtocol` just allows us to register
+     class elements (no instances are possible).
+     */


No need to add documentation here since it's a private var -> implementation detail on how the Router and Server communicate and Server keeps storage of routers, nothing that the client needs to be aware of.

I would like to keep this comment. How shall I mark it that its not visible to the public? /!* ??

Nothing special needed, private prop/methods/types are not accessible anyway. However we try to limit comments , only when really needed

This comment is not clear/correct IMHO. Not sure if it's even needed

Oh important: routers should remain private and yes, I will also say that this comment should be removed.

removed comment in 60417ca

joanromano · 2016-08-12T09:11:47Z

Tests/RouterTests.swift

+        // intentionally left empty
+    }
+}
+


What is this ProtocolClientTest for? I think we shouldn't need to create it.

Basically, the normal way to test it would be:

With any implementation, add the test that cancels the data task and expects the request to not get started. Thus, this test will fail.

Add the implementation to stop the requests in KakapoServer

Check the test again, should pass.

For the test itself: should be enough to get the task from dataTaskWithURL and cancel it, and expect the request to not being called. Same as you do in "should not start a request when request gets cancelled". We could also add other test with more than one request or with one canceled request and another not cancelled request, but that should be it. I don't think we need more tests with a new NSURLProtocolClient

Yes, I noticed the current flaw with the cancelledRequests list on the Router instance. Requests with identical URLs cannot be distinguished with this design. Soooo.... I will go the way that @MP0w has suggested (the flag on the server). I'll adopt the tests accordingly.

I need the ProtocolClientTest to manually simulate the control flow in the KakapoServer (see: "should send notifications when loading has finished").

Because there's no way for me to access the KakapoServer instances, which are created automatically be the OS. Thus I create one KakapoServer instance manually to test drive the start & stop logic.

joanromano · 2016-08-12T09:14:56Z

I'd be out for the weekend until Monday evening, I will check again everything then. @leviathan thanks for all your efforts on making this feature happen 👍

…erver request be marked as cancelled, adopted tests accordingly,

joanromano · 2016-08-16T21:38:48Z

Source/KakapoServer.swift

+
+     Note: calls to `stopLoading()` will set this value to `true`
+     */
+    public private(set) var requestCancelled:Bool = false


I don't think we need this to be publicly readable, as stated before: You shouldn't use this class directly but register aRouterinstead.

Oh nevermind, I just saw the access from the Server.

@joanromano we need contributors and style guidelines... for this time we can just apply the style ourself (e.g. Space missinggn between : and Bool)

Public should be removed, internal getter is fine to access it from the Router

reduced variable visibility and removed comment in 60417ca

joanromano · 2016-08-18T08:38:29Z

@leviathan thanks for the time invested on this. Overall looks good but after double checking last day I think I might have to discuss some implementation details with @MP0w before merging.

I might re think again the scenario of adding all this logic to the Router since it probably makes more sense from the responsibilities perspective (and also for unit testing) though, I also agree, harder to implement and error-prone. I know this is what we have been trying to avoid from the beginning but I somehow changed my mind lately.

I am out two weeks for holidays and so will review this again together when I am back. Sorry for this taking too long and, again, many thanks for your efforts on making this happen.

MP0w · 2016-08-18T15:23:23Z

Tests/RouterTests.swift

+
+            beforeEach {
+                router = Router.register(baseURL)
+                router.latency = latency // very high latency to allow us to stop the request before execution


Since the request is always asynchronous theoretically also 0 latency should work

MP0w · 2016-08-18T15:38:13Z

@joanromano I think is fine , what are your concerns? Other than the few changes we asked I think it's good.

joanromano · 2016-08-18T16:16:14Z

@MP0w I don't like having to change the signature of startLoading to a explicit KakapoServer and also, having to check the server afterwards for the flag.

I think it's just not a proper separation of concerns, and it shows in the tests when we need to manually create a KakapoServer and directly call in it stopLoading and startLoading (when clients are not supposed to interact with the server at all): for me that's a signal that something may be wrong.

MP0w · 2016-08-18T17:11:28Z

That test is just to check that the unit is working as expected ... it's basically a mocked KakapoServer

leviathan · 2016-08-19T14:04:40Z

@joanromano I understand your objections. Because, when Router is mocking away the incoming request, then the latency just tells the Router to wait some time before telling the network delegate that the request is finished.

This is also the reason why I needed to create a KakapoServer instance in the Router unit test myself, because otherwise the incoming request is totally out of control (from the Router perspective).

Right now, I'm a bit confused how to proceed with this. Looks like the two of you have to sync.
Could you advise please how to proceed here...

…on logic, reduced visibility of variables

…case.

MP0w · 2016-08-20T14:17:35Z

@leviathan sorry for the confusion, IMO after fixing those last 2 things it's good to merge, the implementation seems correct and it's working.
@joanromano and me are trying to always find the best solutions in this project because one of the main goal was to learn rather than ship ASAP. That's why we discuss a lot and takes longer than expected. That means that we may reconsider implementations (we already did with our own code) and refactor things and we take this as an opportunity to discuss and learn.
Since ATM this is the best implementation we can think of I will merge after the last 2 changes requested.
When @joanromano comes back from holidays should be free to rethink about it and eventually submit a PR (adding @leviathan as reviewers if he likes).

@leviathan thank you for the time you spent on this, we really appreciate it. I hope you are fine with our approach!

MP0w · 2016-08-20T14:31:20Z

oh weird, these tests:
it("should not confuse multiple request with identical URL")
it("should mark a request as cancelled")

Are succeeding also when commenting requestCancelled = true in KakapoServer... I wonder if all this is needed or is automatically handled by NSURLProtocol ?!?
The tests looks correct

MP0w · 2016-08-20T18:52:48Z

Merged 🎉 ty very much @leviathan

leviathan added 2 commits August 10, 2016 14:26

implemented NSURLProtocol stopLoading method

57e42fa

added IntelliJ AppCode meta file directory to git ignore

2106533

leviathan mentioned this pull request Aug 10, 2016

Implement NSURLProtocol.stopLoading() for delayed requests #88

Closed

leviathan changed the title ~~Master~~ Implement NSURLProtocol.stopLoading() for delayed requests Aug 10, 2016

joanromano reviewed Aug 10, 2016
View reviewed changes

leviathan added 5 commits August 10, 2016 15:20

using indexOf instead of swift array filter to reduce roundtrips

9c98c60

Detailed the code documentation to make the internal NSURLProtocol (O…

a6d40f0

…S) logic better understandable

Fixed spelling mistakes in code docs

2415924

Router now remembers the 'to be canceled' URLs & added router unit te…

c10b110

…sts for 'stopLoading'

Merge remote-tracking branch 'devlucky/master'

2d0203d

Move request cancelation to private method and fixed unit test for Ro…

f5d61a9

…uter (avoid accessing private vars)

joanromano reviewed Aug 11, 2016
View reviewed changes

leviathan added 3 commits August 11, 2016 15:46

Check that the url client does not deliver notifications when the rou…

ea5a349

…ter's request loading gets cancelled.

Added stopLoading tests to cover the cancellation logic

c65872f

Cancel the request immediately

69338ca

joanromano reviewed Aug 12, 2016
View reviewed changes

Using simple flag on the KakapoServer instance to let the OS mark a s…

b91867f

…erver request be marked as cancelled, adopted tests accordingly,

joanromano reviewed Aug 16, 2016
View reviewed changes

MP0w reviewed Aug 18, 2016
View reviewed changes

leviathan added 3 commits August 19, 2016 16:13

Removed code comments, which expose too much of internal implementati…

60417ca

…on logic, reduced visibility of variables

fixed spelling mistake in test description

cd83ba7

Removed error prone initialization of error response objects in test …

1e6d40b

…case.

leviathan added 2 commits August 20, 2016 16:56

fixed duplicate URL request test

1672e55

Merge remote-tracking branch 'devlucky/master'

e6ea7c8

MP0w merged commit 28abbfa into devlucky:master Aug 20, 2016

Implement NSURLProtocol.stopLoading() for delayed requests #96

Implement NSURLProtocol.stopLoading() for delayed requests #96

Conversation

leviathan commented Aug 10, 2016 • edited

General description

Primarily affected components

Depending components

Expected possible Pitfalls

Device and OS test coverage

codecov-io commented Aug 10, 2016 • edited

Current coverage is 99.16% (diff: 100%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joanromano commented Aug 10, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leviathan commented Aug 11, 2016 • edited

MP0w commented Aug 11, 2016

leviathan commented Aug 11, 2016

MP0w commented Aug 11, 2016

Choose a reason for hiding this comment

leviathan commented Aug 12, 2016

MP0w commented Aug 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joanromano commented Aug 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joanromano commented Aug 18, 2016

Choose a reason for hiding this comment

MP0w commented Aug 18, 2016

joanromano commented Aug 18, 2016 • edited

MP0w commented Aug 18, 2016

leviathan commented Aug 19, 2016

MP0w commented Aug 20, 2016

MP0w commented Aug 20, 2016

MP0w commented Aug 20, 2016

leviathan commented Aug 10, 2016 •

edited

codecov-io commented Aug 10, 2016 •

edited

joanromano commented Aug 10, 2016 •

edited

leviathan commented Aug 11, 2016 •

edited

joanromano commented Aug 18, 2016 •

edited