New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

extend userInfo-dict of .DidComplete-Notification to contain responseData of DataTasks #2427

Merged
merged 2 commits into from Mar 5, 2018

Conversation

Projects
None yet
3 participants
@svendr
Contributor

svendr commented Feb 3, 2018

Goals ⚽️

Alamofire currently doesn't allow generic logging of DataTask-Responses via Notifications. This would be especially useful if you use a wrapper around Alamofire like SwaggerClient and can't access or configure the logging directly.

The .DidComplete-Notification already provides access to the URLSessionDataTask via the userInfo-dictionary, but there's no way to get the raw responseData associated with such a DataTask.

AFNetworking implements a similar notification mechanism, but instead also provides access to the responseData via userInfo-dictionary.

Implementation Details 🚧

I've simply extended open func urlSession(_ session: URLSession, task: URLSessionTask, didCompleteWithError error: Error?) to check if the task's delegate is a DataTaskDelegate and if so, put it's data into the userInfo-dictionary of the .DidComplete-notification.

Testing Details 🔍

I've extended SessionDelegateTestCase with 2 proper test-methods.

@jshier

This comment has been minimized.

Contributor

jshier commented Feb 18, 2018

Thanks for the PR! I'm hesitant to include such a piecemeal change when Alamofire's notifications need to be overhauled anyway and that may be part of Alamofire 5. Introduction and use of the feature now means more breakage in the future. But we'll discuss.

@svendr

This comment has been minimized.

Contributor

svendr commented Feb 24, 2018

Hey jshier, thanks for your feedback. May I ask about the roadmap of Alamofire5? How will the notification mechanism be overhauled?

Did you consider a 4.6.1 or 4.7.0 release, where this small change could be included? It would really help at several projects I’m working on.

I’ll be happy to support you re-implementing and / or testing notifications in AF5 if required ;)

@jshier

This comment has been minimized.

Contributor

jshier commented Feb 26, 2018

I'm inclined to add it for now, since AFNetworking seems to have survived with it. My worry is that we could see memory usage increases. Data is copy-on-write I believe, so there may not be an issue.

@svendr, do you mind investigating how the notification affects the memory usage of apps with large responses?

@cnoon, what do you think? Was this considered when adding these notifications initially?

@driemecker

This comment has been minimized.

driemecker commented Mar 3, 2018

Hey @jshier, I've just used Xcode's Memory Gauge to compare memory consumption of the master vs my ResponseBodyInNotification-branch (with registered observer for .DidComplete-Notif) by using the Example App and downloading a 180MB JSON file (https://raw.githubusercontent.com/zemirco/sf-city-lots-json/master/citylots.json). As expected, there has been no difference in memory usage.

@jshier

This comment has been minimized.

Contributor

jshier commented Mar 5, 2018

@driemecker Thanks for the test! This will be part of the 4.7 release. Thanks again!

@jshier jshier merged commit 491cf2f into Alamofire:master Mar 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jshier jshier added this to the 4.7.0 milestone Mar 5, 2018

jshier added a commit that referenced this pull request Mar 5, 2018

extend userInfo-dict of .DidComplete-Notification to contain response…
…Data of DataTasks (#2427)

* extend userInfo-dict of .DidComplete-Notification to contain responseData of DataTasks

* ResponseBodyInNotification: fix unittests sometimes failing on travis ci
@svendr

This comment has been minimized.

Contributor

svendr commented Mar 5, 2018

@jshier thanks for merging this! And 4.7.0 is already released 👍 Much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment