In Test cases, Fix potential memory leaks by use of NSURLSession #250

mikelupo · 2017-05-17T11:57:01Z

NSURLSession documentation states:
“Important: The session object keeps a strong reference
to the delegate until your app explicitly invalidates the
session. If you do not invalidate the session, your app
leaks memory.”

http://tinyurl.com/jnb6642

Checklist

[ X ] I've checked that all new and existing tests pass
[N/A] I've updated the documentation if necessary
[ X] I've added an entry in the CHANGELOG to credit myself

Description

NSURLSessions can leak according to the Apple docs. So I've fixed the tests that use them.
Fixed a test that was failing (not due to my changes). "Task created in a session that has been invalidated"

I've removed some invalid whitespace (caught by our own Gerrit).

Motivation and Context

Memory leaks which can contribute to unexpected and hard to diagnose improper test behavior.

I ran the tests in XCode. Passing 100%.

NSURLSession documentation states: “Important: The session object keeps a strong reference to the delegate until your app explicitly invalidates the session. If you do not invalidate the session, your app leaks memory.” http://tinyurl.com/jnb6642

mikelupo · 2017-05-17T11:58:12Z

I anticipated my CHANGELOG link to the PR incorrectly. I thought it would be 249, turns out it's 250.
Do I need to change that? I can't imagine that all the other PR contributors had to anticipate that like I did. Please advise...

AliSoftware · 2017-05-17T11:59:05Z

@mikelupo Just push a new commit on your branch to fix that link. No need to create a different PR or whatnot, it's just a new commit to add on your fix_memoryLeaks branch ✨ )

AliSoftware

Not that big a deal, but if you could remove any space from empty lines so that the diff could be reduced, that would be awesome!

You should be able to do that in Xcode by going into the Preferences > Text Editing > Editing tab > Check the 2 boxes in the "While editing: …" section. Then validate, and select all the text in this modified file an hit Ctrl-I to make Xcode re-indent your code and remove trailing spaces in whitespace-only lines.

AliSoftware · 2017-05-17T12:00:52Z

CHANGELOG.md

@@ -1,7 +1,9 @@
 # OHHTTPStubs — CHANGELOG

 ## Master
-
+* Fixed potential memory leaks with use of NSURLSession as detected by our devs.


Please add two spaces after the . (like you can see for other entries).

(This is a Markdown-specific stuff that makes the text on the next line actually be rendered in a new line in same paragraph instead of being joined on the same line)

AliSoftware · 2017-05-17T12:03:25Z

OHHTTPStubs/UnitTests/Test Suites/NSURLSessionTests.m

@@ -366,6 +375,7 @@ - (void)test_NSURLSessionCustomHTTPBody

        requestWithBody.HTTPBody = [@"somethingElse" dataUsingEncoding:NSUTF8StringEncoding];
        [[session dataTaskWithRequest:requestWithBody] resume];
+		[session finishTasksAndInvalidate];


Could you fix the indentation here? (use spaces so that we're sure it matches the rest of the code which already uses spaces too). Yeah, I know, that old war again 😄

AliSoftware · 2017-05-17T22:21:49Z

OHHTTPStubs/UnitTests/Test Suites/MocktailTests.m

@@ -45,14 +45,15 @@ - (void)setUp
 {
    [super setUp];
    [OHHTTPStubs removeAllStubs];
-    
+


Would it be possible to get rid of those changes? 🙏 (by using the trick in the Xcode prefs I mentioned you in the last review or even manually of you don't want to change your prefs, that works too)

Basically all empty lines should really be empty and not contain indentation and spaces. I know it's nitpicking and a matter of taste, I don't care for one style or another but I care about consistency throughout the code base 😉

AliSoftware · 2017-05-17T22:22:17Z

OHHTTPStubs/UnitTests/Test Suites/MocktailTests.m

@@ -110,18 +111,18 @@ - (NSHTTPURLResponse *)runLogin
    NSMutableURLRequest *request = [NSMutableURLRequest requestWithURL:url
                                                           cachePolicy:NSURLRequestUseProtocolCachePolicy
                                                       timeoutInterval:60.0];
-    
+


And if course that applies to everywhere it changed here

@AliSoftware, Sorry I missed that. It's fixed now. My Xcode is already up as you mentioned, but I just didn't CTRL-i to repair the indentation on that one file. It's in the new commit of this PR now.

mikelupo · 2017-05-19T15:54:54Z

oh, I think I've been misunderstanding you. You want me to remove all the blank line fixes. Those blank lines had spaces where there should not have. And since I was in those files, I fixed those purposefully. Gerrit (our local code review tool) catches those and they show up as red space. We are diligent to remove "red space" as leaving it in in typically causes merge conflict headaches down the road.
In light of this comment, do you still want that removed?

AliSoftware · 2017-05-19T19:14:34Z

Oh wait… I totally got it the other way around, my bad, you're right!
I actually am now used in all my project to remove all the indentation spaces from empty lines and remove them… like Gerrit suggests and like what you did!

What I didn't realize when I wrote that comment here is that by the time I started writing OHHTTPStubs some years ago, I didn't use that setting/practice yet, and kept those spaces in empty lines at that time (instead of doing the same as you do… and as I do now on all my other projects too 😄). So I misread that diff and actually thought that there were no spaces and you added them… while in fact that was the total opposite 😝

My concern is I want things to be consistent all across the codebase. So I'd agree that we should remove all the spaces from empty lines and since I never realised that it wasn't the case already in that (quite old now) codebase of OHHTTPStubs I never thought of re-auditing it for it 😉

In light of that you have two choices, I'll let you decide which you like best:

Don't remove the spaces in that PR, to focus on the memleak fix, and do a separate PR to fix all the spaces (not only on those files you changed, but everywhere in OHHTTPStubs's source code, demo project, Unit Tests, …)
Make those changes in the current PR, killing two birds in one stone (even if that doesn't split the tasks as clearly but would avoid you reverting the deletion of spaces just to remove them again a commit later). But then again if you keep the changes you did for spaces in that PR, then better apply the same rules and fix that… in the whole codebase then (even the ones you didn't edit to fix that memleak) 😉

I'll let you decide if you prefer to split that in 2 PRs (memleaks + remove spaces from empty lines) or all in that current one. I'd personally prefer two separate PRs, but would understand if it's too much work for you. In any case, just make sure that the spaces are consistent everywhere (and not in one way on some files and another in other files)

AliSoftware · 2017-05-22T13:29:41Z

🎉 Thx again @mikelupo for the fix and taking the time to follow our strict guidelines 😜
Looking forward to the next PR to remove extra whitespace from code 😉

mikelupo · 2017-05-22T13:44:10Z

You're very welcome! Glad to be able to contribute to a really great app. The strict guidelines are what keeps it stable and easy to maintain.
I'll get that whitespace fix in perhaps even today... :)

AliSoftware reviewed May 17, 2017

View reviewed changes

Fix indentation and markdown syntax per code-review.

6767e96

AliSoftware requested changes May 17, 2017

View reviewed changes

Fixing missed indentation repair based on code review

5c96f6a

mikelupo force-pushed the fix_memoryLeaks branch from 82a8cd9 to 5c96f6a Compare May 18, 2017 11:22

Un-fix whitespace

ce1642f

AliSoftware approved these changes May 22, 2017

View reviewed changes

AliSoftware merged commit b118434 into AliSoftware:master May 22, 2017

mikelupo deleted the fix_memoryLeaks branch May 22, 2017 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In Test cases, Fix potential memory leaks by use of NSURLSession #250

In Test cases, Fix potential memory leaks by use of NSURLSession #250

mikelupo commented May 17, 2017

mikelupo commented May 17, 2017

AliSoftware commented May 17, 2017

AliSoftware left a comment

AliSoftware May 17, 2017

AliSoftware May 17, 2017

AliSoftware May 17, 2017

AliSoftware May 17, 2017

mikelupo May 18, 2017

mikelupo commented May 19, 2017

AliSoftware commented May 19, 2017 •

edited

Loading

AliSoftware commented May 22, 2017

mikelupo commented May 22, 2017

In Test cases, Fix potential memory leaks by use of NSURLSession #250

In Test cases, Fix potential memory leaks by use of NSURLSession #250

Conversation

mikelupo commented May 17, 2017

Checklist

Description

Motivation and Context

mikelupo commented May 17, 2017

AliSoftware commented May 17, 2017

AliSoftware left a comment

Choose a reason for hiding this comment

AliSoftware May 17, 2017

Choose a reason for hiding this comment

AliSoftware May 17, 2017

Choose a reason for hiding this comment

AliSoftware May 17, 2017

Choose a reason for hiding this comment

AliSoftware May 17, 2017

Choose a reason for hiding this comment

mikelupo May 18, 2017

Choose a reason for hiding this comment

mikelupo commented May 19, 2017

AliSoftware commented May 19, 2017 • edited Loading

AliSoftware commented May 22, 2017

mikelupo commented May 22, 2017

AliSoftware commented May 19, 2017 •

edited

Loading