Fix OHHTTPStubsProtocol threading

[NSProgrammer]

Per Apple docs https://developer.apple.com/library/prerelease/ios/samplecode/CustomHTTPProtocol/Listings/Read_Me_About_CustomHTTPProtocol_txt.html :

In addition, an NSURLProtocol subclass is expected to call the various methods of the NSURLProtocolClient protocol from the client thread

So, instead of having callbacks happen on a background queue, OHHTTPStubsProtocol need to capture the executing runloop and execute callbacks from that runloop. This is a requirement of NSURLProtocol.

-- Twitter uses OHHTTPStubs for certain unit tests and we have made this change on our local fork. Providing the fix here for the benefit of all.

[AliSoftware]

Just out of curiosity, why not use block directly as the 3rd parameter (as block is already a dispatch_block_t) instead of doing ^{ block() }?

[NSProgrammer]

Yup, it was unnecessary. Fixed.

[AliSoftware]

Thanks, this seems ace! 👍

Do you think you would be able to add a Unit Test for that (to ensure that every delegate method is indeed invoked on the expected thread)?

Also, don't forget to add an entry in the CHANGELOG.md to credit yourself 😃

(Note: no need to create a new PR, new commits to your branch will be added here automatically)

[NSProgrammer]

Totally understand wanting the unit test to go alongside. I doubt I'll
have time though. We use OCMock for testing this, but I don't want to
introduce a dependency and won't have bandwidth for an alternative. I'm
sorry for only going half way here.

On Thu, Apr 2, 2015 at 1:47 PM, AliSoftware notifications@github.com
wrote:

Thanks, seems ace!

Do you think you could be able to add a Unit Test for that (to ensure that
every delegate method is indeed invoked on the expected thread)?

Also, don't forget to add an entry in the CHANGELOG.md to credit yourself
:)

—
Reply to this email directly or view it on GitHub
#96 (comment)
.

[cdzombak]

👍. We're being bitten by what I think is this same issue.

[NSProgrammer]

This issue was being hit quite frequently in our unit tests and this fix completely eliminates it. It only would get triggered about 5% of the time, but when unit tests are run ~500 times per day, those 25 failures are blocking for dev.

[cdzombak]

This issue was being hit quite frequently in our unit tests and this fix completely eliminates it. It only would get triggered about 5% of the time, but when unit tests are run ~500 times per day, those 25 failures are blocking for dev.

@NSProgrammer that sounds exactly like what we're seeing. (cc @Twigz)

[AliSoftware]

I was planning to wait until I got time to add unit tests for that PR before merging it but maybe I'll merge it sooner and add tests later to avoid waiting too long. Will keep you guys posted

[AliSoftware]

Hi guys,

I can't find a way to create a failing Unit Test: I wrote a test that creates an NSURLConnection on a separate thread, and even before applying @NSProgrammer 's PR, the test still passes.

Here is my unit test so far. Do any of you see anything wrong with that test? isn't it testing the right thing? Because it is passing, even before applying that fix so I'm wondering…

Do you guys have an actual example of when this fails for you, so I can get inspiration to write a proper test that would properly avoid regression once this fix is merged?

[cdzombak]

I can't find a way to create a failing Unit Test: I wrote a test that creates an NSURLConnection on a separate thread, and even before applying @NSProgrammer 's PR, the test still passes.

Here is my unit test so far. Do any of you see anything wrong with that test? isn't it testing the right thing? Because it is passing, even before applying that fix so I'm wondering…

Do you guys have an actual example of when this fails for you, so I can get inspiration to write a proper test that would properly avoid regression once this fix is merged?

At my job we're seeing this hang only one in 50 or so times. Assuming the hanging is a direct result of this particular threading bug, it'll be hard to write a consistently-failing unit test for it 😞

That test looks promising. I wonder, though, if it's possible to more directly test this requirement somehow…

In addition, an NSURLProtocol subclass is expected to call the various methods of the NSURLProtocolClient protocol from the client thread

Is there any way for the test to get the NSURLProtocolClient that's being used and assert directly that this holds true, rather than simply checking that the NSURLConnectionDelegate methods are being called from the same thread?

That might be a silly question, but I am not sure exactly how the NSURLProtocolClient is coupled to the NSURLConnectionDelegate.

[AliSoftware]

@cdzombak Mmmmh, good idea, but unfortunately I don't think we can access the NSURLProtocolClient from outside of the NSURLProtocol implementation itself, so it's probably hard to test this directly.

One way to properly test that directly could be to use mocks, e.g. using frameworks like OCMock to mock / add an "expectation" on calls to -[id<NSURLProtocolClient> URLProtocolDidFinishLoading] (and all other NSURLProtocolClient's methods), so that we can intercept those internal (= from OHHTTPStubs.m implementation) calls from outside (= from the Unit Test code). But I'm not sure I want to add a whole dependency framework like OCMock simply to test this case…

Maybe I'll try to swizzle the NSURLProtocolClient methods myself in the Unit Test code, to intercept those calls during testing (which is basically what OCMock does, but just for one test case it seems simpler to swizzle ourselves than to import the whole OCMock framework/dependency just to swizzle for one test case)

[cdzombak]

unfortunately I don't think we can access the NSURLProtocolClient from outside of the NSURLProtocol implementation itself, so it's probably hard to test this directly.

That's what I was afraid of…

One way to properly test that directly could be to use mocks, e.g. using frameworks like OCMock to mock / add an "expectation" on calls to -[id<NSURLProtocolClient> URLProtocolDidFinishLoading] (and all other NSURLProtocolClient's methods), so that we can intercept those internal (= from OHHTTPStubs.m implementation) calls from outside (= from the Unit Test code). But I'm not sure I want to add a whole dependency framework like OCMock simply to test this case…

This is what I was thinking initially…yes, it's heavy-handed, but OCMock does seem like a possible answer. But then I wondered, can OCMock actually mock a method from a protocol like that? That's not something I've done before.

[AliSoftware]

Yeah I agree, given that NSURLProtocolClient is actually a protocol, not a class, I'm not sure either we can actually do that (it's ages I haven't used OCMock personally, maybe I'm wrong).
Will try and investigate swizzling the methods myself, but unfortunately I'm quite sure we can't swizzle methods on a protocol, because we swizzle implementations so we need to know the class that implement it, and in our case it's a internal class private to Apple, but I may try anyway.

@NSProgrammer was talking about their tests using OCMock earlier, maybe he got some pointers on how they did it on their side?

[cdzombak]

Yeah I agree, given that NSURLProtocolClient is actually a protocol, not a class, I'm not sure either we can actually do that (it's ages I haven't used OCMock personally, maybe I'm wrong).

Yeah, I am pretty confident that doing this isn't possible with OCMock (though I would be happy to be proven wrong).

Will try and investigate swizzling the methods myself, but unfortunately I'm quite sure we can't swizzle methods on a protocol, because we swizzle implementations so we need to know the class that implement it, and in our case it's a internal class private to Apple, but I may try anyway.

If you're going to tie the test to whatever private class implements this protocol, it may still be worth considering OCMock. I know it's a lot to add to the project, but swizzling methods yourself can be tedious. Obviously, up to you, but it might be a tradeoff worth considering.

[NSProgrammer]

One option is to apply indirection. You create a custom NSURLProtocolClient implementation that redirects all messages to the NSURLProtocol's client property and instead swizzle/mock the NSURLProtocol's client property. You might even be able to get away with overriding the getter for the "client" property in your NSURLProtocol subclass and instead have the custom NSURLProtocolClient indirect to the super.client instead.

[AliSoftware]

Ah, I like that idea @NSProgrammer will try to implement it ASAP! Thanks for the pointers.

[AliSoftware]

@NSProgrammer I finally mocked the NSURLProtocolClient to intercept all called made to it.
And I was surprised that this revealed me… that your fix didn't actually "call the NSURLProtocolClient method on the client's thread". Well or more precisely, I'm not sure what Apple wanted to refer when talking about the "client's thread" here.

In fact, the -startLoading method is called on an Apple's private thread called com.apple.NSURLConnectionLoader, and not on the thread the NSURLConnection method itself was called on. Thus as you store the CFRunLoopGetCurrent() there, all your calls to NSURLProtocolClient's methods are done on that same runloop… which is associated to that com.apple.NSURLConnectionLoader thread, and not on the thread on which the user did call NSURLConnection in its own code.

I don't know if your code is doing the wrong thing by calling those methods on the com.apple.NSURLConnectionLoader private thread, or if we misunderstood that not in the documentation / example's README that tells us to call those methods from the "client's thread" and that this is what we actually are expected to do (instead of what I initially understood in that sentence, which was calling those methods on the same thread the user did call the NSURLConnection/NSURLSession methods).

Any insight on that?
If what you code do (calling the methods on the com.apple.NSURLConnectionLoader thread) is the right thing, I'll probably end up removing my test and stop trying to test that feature which has proved quite hard to test… and is even maybe not testable at all (I can't see a way to properly capture from my Unit Tests that actual thread/runloop the NSURLProtocol is using internally, and even if it's possible, this will probably need way too complex unit testing for that little feature and won't probably be worth it)

[AliSoftware]

@NSProgrammer I just finally merged your fix into master and pushed version 4.0.1… without any Unit Testing.

I still have my work on attempts of Unit Testing it on the threading-unit-test branch that I'll keep open and around if you have any idea about it.

The question that remains is:

• Are we expected to call the NSURLProtocolClient methods on the com.apple.NSURLConnectionLoader thread on which all methods of the NSURLProtocol instance are called, which is what your fix does
• Or are we expected to call the NSURLProtocolClient methods on the thread on which the NSURLConnection methods have been called by the user/caller, which is what my attempt of a unit test is trying to check (but in that case I don't see how we could achieve that in the code)

[NSProgrammer]

Yes, it is confusing commentary by Apple. The "client thread" they are referring to is the thread that start loading is called from, which is com.apple.NSURLConnectionLoader. I agree it is ambiguous with being potentially the thread the NSURLConnection or NSURLSession is called from, or even the NSRunLoop the NSURLConnection is registered with or the NSOperationQueue the NSURLSession has set as a delegate queue. However, we have confirmed with Apple directly that it is the thread that startLoading is called from. It is an implementation detail that the thread happens to be com.apple.NSURLConnectionLoader, but it makes sense that networking is synchronized via being on a dedicated thread. Sorry for the delay in replying.

[AliSoftware]

Thanks so much for those details and feedback :)

I'll file a feedback on NSURLProtocol's documentation page to ask Apple to add some more details about that, but as you confirmed with Apple that they expect it to be called "on the same thread that startLoading is called from" then that's good enough for me.

4.0.1 is out with your fix already. I'm not sure I'll be properly able to test this properly. I can surely be done somehow, by intercepting the call to startLoading then the calls to all NSURLProtocolClient's method, but it seems to me that it's not worth the effort of writing such complicated test code just for that one feature.

Thanks again

[NSProgrammer]

Sounds good, thanks! And just for reference, most of our NSURLProtocol learnings came from building CocoaSPDY.

https://github.com/twitter/CocoaSPDY

[AliSoftware]

Interesting project! Will take a look, I didn't know about SPDY.

As you seem to have worked a lot with NSURLProtocol, and even talked to Apple engineers about it, maybe you have an idea on issue #82 (-[NSURLSessionConfiguration HTTPAdditionalHeader] not being added when the NSURLProtocol methods are called since iOS8, see related unit test on this branch)?

[NSProgrammer]

I haven't personally made these discoveries or any acquaintances at Apple; that's mostly comes from the core contributors to CocoaSPDY. However, I know from working closely with them (and with NSURLProtocol over the past 10 years) that NSURLProtocol is not a priority for Apple and so things will often be forgotten for compatibility making it unstable and only the most common use cases are maintained (though I argue that's purely coincidence).

For example:
http://openradar.appspot.com/radar?id=5792159408062464
http://openradar.appspot.com/radar?id=5209760232112128

I wouldn't be surprised if the place where HTTPAdditionalHeaders gets added to the canonical headers of the request moved from the NSURL loading system into the private NSURLProtocol implementations that Apple has and they just didn't care about the side effect. What this really boils down to is the core flaw in NSURLProtocol; that the NSURLSessionConfiguration is not available to the protocol. This particularly affects how cookies are handled which differs between NSURLConnection to NSURLSession, but ultimately no configuration properties are available to the protocol.

You can see in the latest addition to CocoaSPDY that I created a patch (submitted vicariously by a coworker) such that the NSURLSession (and therefore the configuration, delegate and delegate queue) can be provided to the SPDYURLProtocol (see the SPDYURLSession in NSURLRequest+SPDYURLRequest on the "develop" branch). This might be an avenue to pursue, though it is admittedly a workaround.

Best of luck, we appreciate your framework and your dedication to keeping it fresh!

[AliSoftware]

Thanks!

Yeah I started a branch with a workaround too, that creates a subclass of OHHTTPStubsProtocol dynamically using ObjC runtime and insert it (instead of the base OHHTTPStubsProtocol class) to the NSURLSession's protocolClasses so I can get that associated session back from the protocol. Seems hacky though.

There is a NSURLProtocol (NSURLSession) category that can be found at the end of NSURLProtocol.h (documented nowhere of course…) but it doesn't even seem to work! (Despite what this category says, NSURLProtocol does not responds to the task selector so that @property declared here does not seem to be implemented at all… :sad: )