Default value for flushAfterIdleTimeMillis

[jorcasso]

Hi, I'm testing the release 0.7 and I have found this diving a bit into the source code of ApnsClient:

private long flushAfterIdleTimeMillis = DEFAULT_WRITE_TIMEOUT_MILLIS;

Shouldn't flushAfterIdleTimeMillis use DEFAULT_FLUSH_AFTER_IDLE_MILLIS instead as default value?

[jchambers]

Yes, it certainly should. I'm not entirely sure how tests are even passing given the extremely long timeout. Obviously, we'll fix the problem, but I'd also like to figure out why tests aren't hanging forever.

[jorcasso]

I have investigated a bit that situation and found that if I sent a message just after connecting the client a flush was triggered in io.netty.handler.codec.http2.Http2ConnectionHandler.channelReadComplete that made it work. But if I waited some seconds after connecting it failed.

I have found, however, that the timeout threshold does not seem to work properly, I set it to 50 millis, but I saw that the flush was done after 60 seconds, just when ping idle timeout happened. I saw that all the write timeout events had isFirst() == false and as I see in the implementation of io.netty.handler.timeout.IdleStateHandler that events will not change to isFirst() == true until a flush is done.

[jchambers]

I have investigated a bit that situation and found that if I sent a message just after connecting the client a flush was triggered in io.netty.handler.codec.http2.Http2ConnectionHandler.channelReadComplete that made it work.

Yeah—I just came to the same conclusion. The test was enqueueing the notification before the initial SETTINGS frame came in from the server; when it arrived, it would trigger a flush.

You're right about the timeout still being ineffective; I addressed that in 66083b1. We should be in good shape now from a functional perspective, but I'm trying to think of a good way to wait for the initial SETTINGS frame to arrive so we'll catch this problem in the future. Thread.sleep seems okay-ish, but it seems like there's probably something a little more robust we can do.

Thanks again for digging into this problem!

[jorcasso]

You're welcome @jchambers, thanks for fixing.

[jchambers]

Added a test that explicitly waits for the initial SETTINGS frame in 24c1b06.

[jorcasso]

I'm thinking that as the channelReadComplete triggers a flush, when the responses from the APNS server are received there will be some extra calls to flush that may prevent to obtain better performance regarding the threshold of maxUnflushedNotifications.

[jchambers]

Maybe so, but we still do see some improvements in local benchmarks.