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

add support for http proxy #129

Closed
wants to merge 5 commits into from
Closed

Conversation

YanKawaYu
Copy link

I have the same problem in this issue.
#24 (comment)
I found that apns2 doesn't support http2 proxy. In order to fix the problem, I made some improvements to the following code.

@deltapath-eric
Copy link

Hi, seem this changes only available in cert client and the token client is kind of missing?

@coveralls
Copy link

Coverage Status

Coverage decreased (-8.3%) to 83.818% when pulling 8890cd1 on zhaoxy2850:master into 060d44b on sideshow:master.

4 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-8.3%) to 83.818% when pulling 8890cd1 on zhaoxy2850:master into 060d44b on sideshow:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-8.3%) to 83.818% when pulling 8890cd1 on zhaoxy2850:master into 060d44b on sideshow:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-8.3%) to 83.818% when pulling 8890cd1 on zhaoxy2850:master into 060d44b on sideshow:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-8.3%) to 83.818% when pulling 8890cd1 on zhaoxy2850:master into 060d44b on sideshow:master.

@coveralls
Copy link

coveralls commented May 16, 2019

Coverage Status

Coverage decreased (-1.9%) to 90.182% when pulling 37c5474 on zhaoxy2850:master into 060d44b on sideshow:master.

@emiksk
Copy link

emiksk commented Nov 21, 2019

Will this PR be merged?
I wish this feature is implemented to the library.

@emiksk
Copy link

emiksk commented Dec 3, 2019

Hi, @sideshow

I want to add this feature in this library.
What do I should do to merge this pull request or add this feature?

@sideshow
Copy link
Owner

sideshow commented Dec 4, 2019

@zhaoxy2850 @emiksk

I agree that there is definitely a need for apns2 to support proxy servers but I am hesitant to merge this as it is because of the following;

  1. Its adds two more methods NewProxyClient and NewProxyTokenClient so now we have 4 client constructors. I think a better way is to allow either setting a default Transport/Httpclient or allowing you to pass a custom one into the constructor as i am sure there will be more customisations that others want to do to the transport/httpclient and this will avoid us having to add more methods each time.

  2. This uses http2.ConfigureTransport which takes a http1 connection and upgrades it to http2 whereas the other methods used a pure http2 connection. I would rather use the http2 transport directly however the http2 transport doesn't yet support http2 proxying.

For now I have added an example of how to use the proxy code you have in this PR with master as it is today; Its a little bit more code to get set up but it does work and means you can use with a proxy. Please try this and let me know how you get on. Thanks

@sideshow sideshow mentioned this pull request Dec 4, 2019
@YanKawaYu
Copy link
Author

Thanks. This can be an alternative way until http2 transport support http2 proxying.

@YanKawaYu YanKawaYu closed this Dec 4, 2019
@emiksk
Copy link

emiksk commented Dec 5, 2019

@sideshow
Thank you so much for your explanation and support.
I understand your opinion. I'll follow the example!

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.

None yet

5 participants