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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The response of sendNotification may not fulfill #725

Closed
ylgrgyq opened this issue Oct 14, 2019 · 6 comments · Fixed by #742
Closed

The response of sendNotification may not fulfill #725

ylgrgyq opened this issue Oct 14, 2019 · 6 comments · Fixed by #742
Labels

Comments

@ylgrgyq
Copy link
Contributor

ylgrgyq commented Oct 14, 2019

Hi jchambers,

I thought I found another issue. Please take some time to review it when you are available.

It seems the response promise for sendNotification may stay in unfinished state forever when write failure occurs here or here. Because there's no listener on writePromise to pass any exceptions it got to responsePromise.

I think maybe we can fix this by changing the block start at line 214 in ApnsClient to:

channel.writeAndFlush(responsePromise).addListener(new GenericFutureListener<ChannelFuture>() {

    @Override
    public void operationComplete(final ChannelFuture future) throws Exception {
        if (future.isSuccess()) {
            ApnsClient.this.metricsListener.handleNotificationSent(ApnsClient.this, notificationId);
        } else {
            responsePromise.tryFailure(future.cause());
        }
    }
});

And removes the tryFailure here.

What do you think?

Sorry for not taking PR right now because I may be missing something. So if you agree with the modifications above, I would love to take a PR as soon as possible.

By the way, my enviroment is :
jdk: 1.8.0_192
pushy: 0.13.10
netty: 4.1.37.Final
tcnative: netty-tcnative-boringssl-static-2.0.25.Final-linux-x86_64

Thanks.

@jchambers
Copy link
Owner

At a quick glance, I think you may be right. I'll analyze the situation more carefully as soon as I can. Thanks kindly for the report!

@barkefors
Copy link

I would bumbly like to "bump" this, if time permits.

@ylgrgyq: Are you running with the PR changes in production and seeing a difference? We've recently started observing a similar behavior once our "batches" reached a certain volume.

@ylgrgyq
Copy link
Contributor Author

ylgrgyq commented Dec 5, 2019

@barkefors :
Yes, I tried my patched version and did not see any pending response future at least during the testing period. But we have reverted to Pushy 0.13.10 because this is a comparatively minor issue for us and we need to stick on the release version of our dependencies to package services in our environment.

@jchambers
Copy link
Owner

Folks, I expect to be able to focus on this in a few weeks, and I regret that I haven't been able to give it more attention so far.

@jchambers
Copy link
Owner

I have a fix in the works and hope to have it up for review by the end of the day. Thank you for your patience!

@jchambers
Copy link
Owner

I've opened #742, which should fix this issue. Your feedback is welcome and greatly appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants