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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: DownloadRequest event duplication and missing events. #2807

Merged
merged 6 commits into from Apr 16, 2019

Conversation

Projects
None yet
2 participants
@jshier
Copy link
Contributor

commented Apr 14, 2019

Issue Link 馃敆

This PR covers a variety of areas, but should also fix the DownloadRequest part of #2759.

Goals 鈿斤笍

This PR fixes a few issues:

  • DownloadRequest overrides cancel() but didn't get the previous refactor. This PR applies the previous strategy to the overridden cancel() to fix duplicate event calls.
  • DownloadRequest was missing event calls for response serialization. These have been added.
  • Bizarrely, DownloadRequest gets metrics and task completion in a different order than DataRequest/UploadRequest, which meant that metrics were never actually being gathered, as the task / request map was broken as soon as the task completed but before metrics were gathered. Additional state and API has been added to RequestTaskMap to track both events and only break the association when both have been received.
  • Duplicate task events could be received when tasks had already transitioned to a new state but the action's work was still enqueued. Logic has been added to ensure a task can be transitioned to a state before doing so. This should ensure that all task transitions happen only once.

Testing Details 馃攳

This PR adds tests for all request types to ensure all lifetime events are properly being called, no more, no less.

@cnoon cnoon self-requested a review Apr 15, 2019

@cnoon cnoon added this to the 5.0.0-beta.6 milestone Apr 15, 2019

@cnoon
Copy link
Member

left a comment

Looking good overall @jshier. A couple items back to you.

@@ -28,14 +28,18 @@ import Foundation
struct RequestTaskMap {
private var tasksToRequests: [URLSessionTask: Request]
private var requestsToTasks: [Request: URLSessionTask]
private var taskEvents: [URLSessionTask: (completed: Bool, metricsGathered: Bool)]

This comment has been minimized.

Copy link
@cnoon

cnoon Apr 15, 2019

Member

I think I'd recommend making a TaskEvent struct with completed and metricsGathered properties to make the switches easier to follow. Right now you're switching on true and false values without enough context.

This comment has been minimized.

Copy link
@jshier

jshier Apr 15, 2019

Author Contributor

Ah, that would make the calculations easier.

This comment has been minimized.

Copy link
@jshier

jshier Apr 15, 2019

Author Contributor

Actually, looking into it, it doesn't really make the calculations any easier. What exactly did you have in mind? AFAICT, I'll need to switch on the booleans either way.

This comment has been minimized.

Copy link
@cnoon

cnoon Apr 15, 2019

Member

It would just make the switch more clear:

switch events {
case (true, _): fatalError("RequestTaskMap consistency error: duplicate completionReceivedForTask call.")
case (false, false): taskEvents[task] = (completed: true, metricsGathered: false)
case (false, true): self[task] = nil
}

vs.

switch (events.completed, events.metricsGathered) {
case (true, _): fatalError("RequestTaskMap consistency error: duplicate completionReceivedForTask call.")
case (false, false): taskEvents[task] = (completed: true, metricsGathered: false)
case (false, true): self[task] = nil
}

Right now, I have no idea what's on the left and what's on the right when looking at the switch.

This comment has been minimized.

Copy link
@jshier

jshier Apr 15, 2019

Author Contributor

Okay, I can do that.

Show resolved Hide resolved Source/Session.swift
Show resolved Hide resolved Source/RequestTaskMap.swift

@jshier jshier merged commit 63cbd53 into master Apr 16, 2019

1 check failed

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

@jshier jshier deleted the bugfix/event-consistency branch Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.