Hey people, any thoughts about this? Seems quite simple.

@phoney You are right about that case. Although I decided to fix it as in 8b664b0, because the sooner we deallocate self.mutableData, the better. What do you think?

That was weird, is like Travis never even started.

This is the Travis log:

No output has been received in the last ... minutes, this potentially indicates a stalled build or something wrong with the build itself.
The build has been terminated

@kcharwood I've seen you are taking good care of this project. This seems like a pretty much straightforward PR, could you please review it? I'm not sure who can decide whether to merge it or not. Thanks.

Looks OK now.

+1

@gfzabarino That travis log has been the death of me. It's just appears to be totally random and we have to kick it back off.

I'll take a look at this one this week. I've been consumed with 2702, which I think we are almost out of the woods on. It's super pressing to get that one out the door, then I'll circle back on these.

🍻

Also, @gfzabarino do you think we need any additional unit tests? I would love to see tests with just about every PR we try to get in going forward. Just to make sure we prevent as many regressions as possible

@kcharwood I guess I could write tests to make sure wherever final NSData is passed it is always the same instance. But if in the future someone writes a new way of delivering the downloaded NSData to the client, I have no way of testing for that, since it is still not coded :) Having said that, would you like me to still code some tests about the current implementation?

I'd be interested in at least seeing your ideas on how it could be tested and verified.

Hey @gfzabarino

I apologize for just now getting to this. I'm ready to make it happen.

I did a slight refactor here: 79a8d58

The two issues I wanted to address were removing setting two variables on the same line, and getting that copy into one place instead of two. How's that look to you? Ready to merge if you give the thumbs up.

Hi @kcharwood. One small comment. In the original implementation (check 29da5e5), if there's an error, the dictionary entry with AFNetworkingTaskDidCompleteResponseDataKey key is still provided in the dictionary. It appears this doesn't happen anymore with your changes, what do you think?
Also, FYI I've tried to think to use the NSData dataWithBytesNoCopy family of initializers to see if there was a way to prevent having a spike of memory in which both variables hold the same data, even if it is in a small period of time. Yet, I wasn't able to come to a solution using this. The only thing that occurred to me was to init the data to include in the response dictionary with 'no copy', and do not nil the mutableData property, but then we won't be able to release the data until the whole AFURLSessionManagerTaskDelegate instance is released. Do you have any thoughts about this?

Good catch @gfzabarino

How about 1167d09?

Yup that looks like 👍 to me @kcharwood. Thanks for wrapping this up.

Merged in with 1167d09

Thanks!

🍻