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

Removes the old request to avoid endless growth. #2412

Closed
wants to merge 5 commits into
base: bug/2411-retry-leaks
from

Conversation

Projects
None yet
4 participants
@mrdepth
Copy link

mrdepth commented Jan 9, 2018

Issue 2411

Implementation Details

Remove the old request from delegate.protectedRequests before assigning new one.

Testing Details 馃攳

class Retrier: RequestRetrier {
	func should(_ manager: SessionManager, retry request: Request, with error: Error, completion: @escaping RequestRetryCompletion) {
		completion(request.retryCount < 5, 0)
	}
}

var session: SessionManager? = SessionManager()
session?.retrier = Retrier()
session?.request("http://google.com")
	.validate(statusCode: 400...500) //simulate error
	.responseString { (response) in
		//session.delegate.protectedRequests contains 5 records
		session = nil
}

jshier and others added some commits Dec 4, 2017

Decodable response serialization and major refactor (#2265)
* First draft of response serializer refactor and decodable serializers.

* Refactor serializer protocols and implementations.

* Finish refactors, update inline docs.

* Remove download serializer from Data, as it鈥檚 default now.

* Update whitespace.

* Add failure expectation test.

* Remove unnecessary assert.

* Refactor serializers to use throws instead of Result.
Refactor Lock Usage and Introduce Protector (#2290)
* Drop iOS 8 / macOS 10.10 support and remove all workarounds.

* First draft of response serializer refactor and decodable serializers.

* Refactor serializer protocols and implementations.

* Finish refactors, update inline docs.

* Remove download serializer from Data, as it鈥檚 default now.

* Update whitespace.

* Add failure expectation test.

* Initial versions of Mutex and Protector.

* Rename value to unsafeValue.

* Add UnfairLock.

* Use UnfairLock on supported OSes, hide everything.

* Clean whitespace.

* Cleanup based on comments.

* Remove TODO.

@jshier jshier requested review from jshier and cnoon Feb 18, 2018

@jshier jshier added this to the 4.7.0 milestone Feb 18, 2018

@jshier jshier added the needs tests label Feb 18, 2018

@jshier

This comment has been minimized.

Copy link
Contributor

jshier commented Feb 18, 2018

Thanks for the PR! This looks straightforward, but I'd like @cnoon to review as well.

Also, we need some tests around this to ensure the requests don't grow on retry.

@jshier
Copy link
Contributor

jshier left a comment

Tests need to be added but otherwise it looks good to me.

@jshier jshier modified the milestones: 4.7.0, Next Mar 5, 2018

@oadk

This comment has been minimized.

Copy link

oadk commented Apr 13, 2018

Hi,

Do you have a rough timescale for when this PR might be merged? We have unfortunately been hit by this problem, and it is causing unbounded memory growth when there are lots of retried requests (e.g. on poor quality mobile connections). We can patch locally, but it would be great to see the fix in an official release.

(Thank you for making Alamofire - it's awesome!).

@jshier jshier changed the base branch from alamofire5 to master Apr 15, 2018

@jshier jshier changed the base branch from master to alamofire5 Apr 15, 2018

@cnoon

cnoon approved these changes Apr 15, 2018

Copy link
Member

cnoon left a comment

Thank you for putting this together @mrdepth...much appreciated! 馃嵒

@cnoon

This comment has been minimized.

Copy link
Member

cnoon commented Apr 15, 2018

@jshier this is absolutely something we need to get resolved ASAP. I'd recommend we move this through on this branch, then cherry pick the change back to master along with some tests.

There's quite a few ways we could test this, but it will require a retrier to keep retrying (maybe up to say 2 or 3 times), and then stop retrying. Then we just need to run a count on the session delegate's number of requests.

If you have time to take that on awesome! Otherwise I can take a pass tomorrow.

@jshier jshier changed the base branch from alamofire5 to bug/2411-retry-leaks Apr 15, 2018

@jshier jshier closed this Apr 15, 2018

@jshier jshier referenced this pull request Apr 15, 2018

Merged

Fix Retrier Leaks #2491

@oadk

This comment has been minimized.

Copy link

oadk commented Apr 16, 2018

Thanks everyone for the patch and 4.7.2 release. This is very helpful.

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.