Remove pending acquirePromise once chained responseFuture fails

[edwardxia]

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 retained acquirePromise effectively becomes useless. Even if it recovers and resolves later, the next chained responseFuture is already resolved as failure, and the listener on the acquirePromise effectively becomes a no-op when attempting to complete a responseFuture that already failed.

Holding the acquirePromise in memory also holds a Listener object, which hold the responseFuture, which holds an Exception, which holds a StackTrace, etc... The total retained memory foot print for just a single acquirePromise is about 1.27KB in a failure case.

With that said, as soon as responseFuture fails, there is no reason for keeping its acquirePromise, therefore we can get rid of it:

1. Listen on responseFuture's failure, and attempt to cancel acquirePromise if it is cancellable (not resolved yet).
2. Listen on acquirePromise's cancel event, and remove it from pendingAcquisitionPromises to allow GC to collect these unnecessary pending requests immediately.

[jchambers]

Hello, and thank you for the contribution! Please accept my apologies for the delayed response.

To get right to the core argument:

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.

It is always the caller's responsibility to remain in control of how many unresolved notifications are "in flight." The specific mechanism for potential failures doesn't matter. Callers must always be prepared for "send notification" calls to be slow or fail and must manage their own consumption of system resources. While I understand where you're coming from, I don't think this argument changes anything from #1095.

I do think it's a good point that we might be able to optimize a bit by removing elements from the channel pool's acquisition queue when the associated Future is cancelled. That said, I think this pull request proposes cancelling Futures after acquition has—by definition—completed, so I think it's a no-op as written. I may be misunderstanding something, but I just don't see how the proposed changes in ApnsClient could ever actually have an effect. Please let me know if you disagree and if I'm missing something!

I don't think it makes sense to merge this as it's currently framed both because (a) callers need to be managing flow control anyhow and (b) I don't think these specific changes would work as intended.

I'd be open to merging just the "remove from queue on cancellation" change if you'd like to reframe this pull request around that specifically, but I'd also insist upon unit tests showing that it works as intended.

Again, thank you for the contribution!

[jchambers]

This doesn't make sense to me; if we're in the acquirePromise's completion listener, doesn't that by definition mean that the acquirePromise has completed by some means and is therefore not cancellable?

[edwardxia]

acquirePromise would hang forever when apple server is not reachable. because these promises do not have any timeout on them. responseFuture timeout would not complete the promise it listens on.

[jchambers]

these promises do not have any timeout on them.

They do if you configure a timeout, right?

pushy/pushy/src/main/java/com/eatthepath/pushy/apns/ApnsChannelFactory.java

Lines 88 to 89 in 7bd7c80

	clientConfiguration.getConnectionTimeout().ifPresent(timeout ->
	this.bootstrapTemplate.option(ChannelOption.CONNECT_TIMEOUT_MILLIS, (int) timeout.toMillis()));

[edwardxia]

Netty has CONNECT_TIMEOUT_MILLIS default 30s when it's not configured, but we saw these promises stuck in memory forever until JVM crashes. This option is likely not working.

I don't have time to investigate but this might be related: netty/netty#9399

[jchambers]

This option is likely not working… I don't have time to investigate but this might be related: netty/netty#9399

The issue you linked stems from somebody blocking their event loop, and I don't think it's relevant here. I also don't think it's reasonable to conclude that connection timeouts aren't working based on the evidence we've seen here. I want to be clear that I believe you had a problem and I'm interested in figuring out what went wrong, but this doesn't seem like the explanation.

Taking a step back, I think we've gotten a little sidetracked. Again, emphasizing that it would be good to understand and fix whatever went wrong for you, I still don't understand this proposed fix. The key problem for me is that it appears that we're trying to cancel the acquisition promise after it has already completed. Do you agree with that assessment? If not, can you please explain why?