Skip to content

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 nebiros force-pushed the fix-unrecognized-selector branch from c9f9cba to 7b175e5 Compare April 9, 2018 16:38
@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