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

Generic HTTP/2 workers #61

Closed
wants to merge 35 commits into from
Closed

Conversation

rslota
Copy link

@rslota rslota commented Mar 6, 2017

This PR introduces a generic HTTP/2 workers that deduplicate lots of code from FCM and APNS workers. The FCM implementation is from #52 .
Also I've hanged HTTP/2 client library. This is due to fact that Kadabra is kinda slow and also, fails to deliver all messages when there are lot of streams open. In my load testing Kadabra showed many flaws, like dropped massages, corrupted connections, etc., while chatterbox is almost flawless (it need one small change: joedevivo/chatterbox#106).

This PR is more like "to discuss" and may require more work.

cstar and others added 30 commits January 10, 2017 16:04
Sync push only, one regid per notification, ugly code, tests not passing.
Some breaking changes in the on_response possible parameters. The return values try to be helpful.

Batch not done yet with over 1000 registration ids.
This test case should fail.
# Conflicts:
#	lib/pigeon/gcm.ex
`on_response` is now passed `%Pigeon.GCM.NotificationResponse`
# Conflicts:
#	lib/pigeon/gcm_worker.ex
@hpopp
Copy link
Member

hpopp commented Mar 6, 2017

Nice. I was thinking about this exact refactor the other day.

I still need to look things over but first things that come to mind:

We used chatterbox up until hex.pm banned all GitHub dependencies. I wrote kadabra as sort of a stop-gap measure until something better came along. I noticed someone finally posted chatterbox to hex, but we'll need to test. It looks like it has very few downloads.

If we do migrate back to chatterbox we also need to make sure heartbeat pings work. chatterbox doesn't keep connections alive on its own, and I remember having extreme difficulty getting it to send a ping manually.

@rslota
Copy link
Author

rslota commented Mar 7, 2017

Hi,

In regard to low download count: this is na erlang project and erlang world still either don't know about hex or simply don't use rebar3 that allow to do that. In my load testing I've tested several HTTP/2 client libraries and chatterbox is the only one that works stable and fast.
If you really need ping feature I can look into it and make a PR to chatterbox that enables that.

@rslota
Copy link
Author

rslota commented Mar 7, 2017

chatterbox ping, here you go: joedevivo/chatterbox#107

@rslota rslota force-pushed the generic_h2_workers branch from 49c6a2a to 087bb38 Compare March 7, 2017 15:39
@asgoel
Copy link
Contributor

asgoel commented Mar 10, 2017

@hpopp couldn't you also get around the chatterbox issue by not including it as an explicit dependency but forcing people who want to use pigeon to add it separately as a dep/application? There are definitely other libraries that do that as well to circumvent the hex.pm issue

@hpopp
Copy link
Member

hpopp commented Mar 14, 2017

That workaround still feels hackish and goes against my general philosophy for published packages.

I wouldn't be opposed to some sort of configuration option that lets you specify the http2 client used. I'm not ready to give up on kadabra yet, but if others want to use chatterbox it should be perfectly acceptable.

@tsloughter
Copy link

I published chatterbox to hex and happy to keep cutting releases when updates you need get accepted into chatterbox.

@hpopp
Copy link
Member

hpopp commented Mar 17, 2017

Awesome! The only outstanding required feature is joedevivo/chatterbox#107. APNS will drop long-running connections if there's no activity.

@tsloughter
Copy link

Ah yea, merging that now.

@tsloughter
Copy link

Merged and I published version 0.4.2 to hex.

@hpopp
Copy link
Member

hpopp commented Mar 23, 2017

Wrote an initial implementation of client adapters at #67. From my testing so far both of the kadabra and chatterbox adapters work fine.

I'm starting to lean more toward making chatterbox the default adapter. I want to update kadabra to use :gen_statem and the Registry, but it will bump the minimum required elixir version to 1.4.

@@ -2,95 +2,42 @@ defmodule Pigeon.APNSWorker do
@moduledoc """
Handles all APNS request and response parsing over an HTTP2 connection.
"""
use GenServer
use Pigeon.GenericH2Worker, ping_interval: 600_000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rslota any chance you can make this ping_interval configurable? some networks (such as AWS) require activity at a higher frequency so the ping has to be lower

@asgoel
Copy link
Contributor

asgoel commented Mar 31, 2017

@rslota how stable is this code compared to master? wondering if i could switch a production app to using this vs kadabra

@hpopp
Copy link
Member

hpopp commented Apr 1, 2017

Updated to v0.12.0 which allows optional chatterbox configuration. Check the README for details

@hpopp
Copy link
Member

hpopp commented Oct 1, 2017

Closing with the published changes in v1.1

@hpopp hpopp closed this Oct 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants