-
Notifications
You must be signed in to change notification settings - Fork 87
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
Fix unrecognized selector error for 'taskIdentifier' and 'originalRequest' methods #77
base: master
Are you sure you want to change the base?
Conversation
Fix unrecognized selector error for 'taskIdentifier' and 'originalRequest' methods
Hm. Is there more context around what the bug is here? I see you mentioned that there's an unrecognized selector error, but what causes it? I'd be hesitant to move forward with merging this without knowing who/what is calling on those selectors. |
@eliperkins ouch, sorry, fix this: #41 (comment), to integrate DVR with Alamofire, I made a little code snippet about it: https://coderwall.com/p/sd7i2g/my-alamofire-dvr-recipe-for-network-testing-in-swift |
@eliperkins any update? |
I'd love to get other folks eyes on this before merging it, as I don't maintain an up-to-date DVR test suite anymore. @soffes @hyperspacemark or other contributors, would you mind reviewing this as well? |
Hi @nebiros , could we update this PR with a description of the problem, the expectation of performance, then how the fix tackles that? |
@dmiluski sure, I update the description |
…' methods - Adds 'taskIdentifier' and 'originalRequest' methods for SessionUploadTask and SessionDownloadTask
c9f9cba
to
7b175e5
Compare
@dmiluski @eliperkins ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the correct approach to take to implement this.
According to the URLSessionTask
docs:
This value is unique only within the context of a single session; tasks in other sessions may have the same
taskIdentifier
value.
Taking a look at how this is implemented in swift-corelibs-foundation
, it looks like the approach taken is to allow the session to hold on to the task identifier counter, and to dependency inject it to each task, using an internal
initializer.
To me, this is the more correct approach, and one that would not allow for task identifiers to clash, should you reach a sufficient amount of them.
Adds support to integrate DVR with Alamofire. Fix: #41 (comment).
I made a little code snippet about it: https://coderwall.com/p/sd7i2g/my-alamofire-dvr-recipe-for-network-testing-in-swift