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

Refactor Request Event Management #2796

Merged
merged 3 commits into from Apr 12, 2019

Conversation

Projects
None yet
2 participants
@jshier
Copy link
Contributor

jshier commented Apr 11, 2019

Issue Link 馃敆

This PR is an alternate take on #2779 and should fix #2759.

Goals 鈿斤笍

This PR refactors how Request's lifetime events are managed.

Implementation Details 馃毀

This PR achieves greater safety and accuracy with a few changes:

  • State transitions inside of Request have been made fully thread-safe through convenience API on Protector.
  • Session's implementation of didResumeRequest, didCancelRequest, didCancelRequest(_:byProducingResumeData:), and didSuspendRequest have been refactored to separate the Request and URLSessionTask lifetime management, ensuring the Request methods are only called once.
  • Session's resumeOrSuspend method has been refactored and renamed to updateStatesForTask(_:request:), which now handles all state combinations between request.state and stateRequestImmediately to ensure only the appropriate actions are taken in certain circumstances.

Testing Details 馃攳

Tests have been added to ensure that only the expect events happen in particular scenarios, no matter how many times resume, suspend, or cancel are called; that the events happen only the correct number of times; and that unexpected lifetime events don't happen.

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

@cnoon cnoon added this to the 5.0.0-beta.5 milestone Apr 11, 2019

@cnoon cnoon added the request label Apr 11, 2019

@cnoon

cnoon approved these changes Apr 11, 2019

Copy link
Member

cnoon left a comment

@jshier I think this PR is awesome. I don't have any concerns, but would love to get your feedback on my race condition comment just to make sure we're on the same page there before we push this through.

Show resolved Hide resolved Tests/RequestTests.swift
Show resolved Hide resolved Alamofire.xcodeproj/project.pbxproj
Show resolved Hide resolved Source/Session.swift

@cnoon cnoon merged commit 5bedc2a into master Apr 12, 2019

1 check failed

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

@cnoon cnoon deleted the bugfix/state-transitions branch Apr 12, 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.