Remove pending acquirePromise once chained responseFuture fails #1115
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #1094.
First, I have read #1095, and I understand why it was rejected. I agree having a limit on pending requests is out of scope.
This PR addresses the "memory leak" in a different way.
Despite that the "memory leak" is "accounted for" and "recoverable", there is still a risk of full service meltdown if the service has very high thoughput and the retained memory grows quickly towards HeapOutOfMemory. Although most likely this will be a user error like a bad certificate etc, the possibility of Apple Server having an outage cannot be entirely ruled out. Having the possibility of APNS server going down causing a client service to have HeapOutOfMemory and bring down that whole service is not acceptable.
In the failure path, if the chained
responseFuturealready fails (e.g. with TimeoutException), the retainedacquirePromiseeffectively becomes useless. Even if it recovers and resolves later, the next chainedresponseFutureis already resolved as failure, and the listener on theacquirePromiseeffectively becomes a no-op when attempting to complete aresponseFuturethat already failed.Holding the
acquirePromisein memory also holds aListenerobject, which hold theresponseFuture, which holds anException, which holds aStackTrace, etc... The total retained memory foot print for just a singleacquirePromiseis about 1.27KB in a failure case.With that said, as soon as
responseFuturefails, there is no reason for keeping itsacquirePromise, therefore we can get rid of it:responseFuture's failure, and attempt to cancelacquirePromiseif it is cancellable (not resolved yet).acquirePromise's cancel event, and remove it frompendingAcquisitionPromisesto allow GC to collect these unnecessary pending requests immediately.