Remove pending acquirePromise once chained responseFuture fails #1115
+16
−1
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
responseFuture
already fails (e.g. with TimeoutException), the retainedacquirePromise
effectively becomes useless. Even if it recovers and resolves later, the next chainedresponseFuture
is already resolved as failure, and the listener on theacquirePromise
effectively becomes a no-op when attempting to complete aresponseFuture
that already failed.Holding the
acquirePromise
in memory also holds aListener
object, which hold theresponseFuture
, which holds anException
, which holds aStackTrace
, etc... The total retained memory foot print for just a singleacquirePromise
is about 1.27KB in a failure case.With that said, as soon as
responseFuture
fails, there is no reason for keeping itsacquirePromise
, therefore we can get rid of it:responseFuture
's failure, and attempt to cancelacquirePromise
if it is cancellable (not resolved yet).acquirePromise
's cancel event, and remove it frompendingAcquisitionPromises
to allow GC to collect these unnecessary pending requests immediately.