E52F SessionDataTask's resume does work on a new background thread... by dasmer · Pull Request #26 · venmo/DVR · GitHub
[go: up one dir, main page]

Skip to content

SessionDataTask's resume does work on a new background thread...#26

Merged
dasmer merged 2 commits intomasterfrom
daz/queue
Sep 23, 2015
Merged

SessionDataTask's resume does work on a new background thread...#26
dasmer merged 2 commits intomasterfrom
daz/queue

Conversation

@dasmer
Copy link
Contributor
@dasmer dasmer commented Sep 21, 2015

as opposed to the calling thread, in order to better mimic NSURLSessionDataTask functionality

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I stopped to wonder if we should call this all in this queue or just call the completion callback in this queue. I think both cases are valid and both accomplish the same thing, but putting all this in a block seems like a bigger hammer than needed, but probably simulates closer to what NSURLSession might be doing.

I'm curious what might be best!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I agree that its a bigger hammer than needed. I'll update accordingly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if either is better, TBH. I think doing all this work on another queue might better simulate a network request.

Just wanted to raise my thought process, honestly.

as opposed to the calling thread, in order to better mimic NSURLSessionDataTask functionality
@dasmer
Copy link
Contributor Author
dasmer commented Sep 21, 2015

@eliperkins We now call the completion on the queue only.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we create a new queue for every data task? Would it may be better to have just one static queue on which these tasks run?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought NSURLSession's queue's were nondeterministic so I wanted to replicate that here. Performance really is not really too important here, since we're only using this in tests (and the drop in performance of creating a new queue on each request is probably insignificant)

That said, I'm open to creating a static queue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure which aspect of the queue you mean when you say non-deterministic. Either way, a session manages just one queue for its lifetime so I think mirroring that within our own Session object would make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a session manages just one queue for its lifetime so I think mirroring that within our own Session object would make sense.

TIL. Updated here: 0262a86

dasmer added a commit that referenced this pull request Sep 23, 2015
SessionDataTask's resume does work on a new background thread...
@dasmer dasmer merged commit 48059cc into master Sep 23, 2015
@dasmer dasmer deleted the daz/queue branch September 23, 2015 14:38
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

0