Skip to content
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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

nebiros
Copy link

@nebiros nebiros commented Jan 11, 2018

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

Fix unrecognized selector error for 'taskIdentifier' and 'originalRequest' methods
@eliperkins
Copy link
Contributor

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.

@nebiros
Copy link
Author

nebiros commented Jan 12, 2018

@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

@nebiros
Copy link
Author

nebiros commented Jan 16, 2018

@eliperkins any update?

@eliperkins
Copy link
Contributor

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?

@dmiluski
Copy link
Contributor

Hi @nebiros , could we update this PR with a description of the problem, the expectation of performance, then how the fix tackles that?

@nebiros
Copy link
Author

nebiros commented Mar 13, 2018

@dmiluski sure, I update the description

…' methods

- Adds 'taskIdentifier' and 'originalRequest' methods for SessionUploadTask
and SessionDownloadTask
@nebiros
Copy link
Author

nebiros commented Jun 22, 2018

@dmiluski @eliperkins ping

Copy link
Contributor

@eliperkins eliperkins left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants