mwkirk / javapns

Test import of svn javapns repo from Google Code
3 stars 0 forks source link

Proxy settings impacting other running processes in JVM #91

Closed mwkirk closed 11 years ago

mwkirk commented 11 years ago

Original author: petend...@gmail.com (December 07, 2011 16:37:20)

What steps will reproduce the problem? 1.Set up JavaPNS to use proxy

What is the expected output? What do you see instead? I would have hoped that the requesting the use of the proxy would only impact the JavaPNS process. Because you are setting the system properties for https.proxyHost and https.proxyPort, that is in effect for other processes that are run in the JVM as well.

What version of the product are you using? On what operating system? v2.1 on Windows XP.

Please provide any additional information below. There are numerous ways this could be handled. A couple that come immediately to mind are: 1) Keep the setting of the proxy host and proxy port internal to JavaPNS by setting internal variables with the value and not system properties. 2) Set JavaPNS-specific system properties that only JavaPNS will retrieve (e.g. "javapns.https.proxyHost").

Original issue: http://code.google.com/p/javapns/issues/detail?id=91

mwkirk commented 11 years ago

From sype...@gmail.com on December 13, 2011 14:48:37 Fixed in r342: proxy can now be set on AppleNotificationServer and AppleFeedbackServer directly to avoid impacting other JVM processes.

mwkirk commented 11 years ago

From petend...@gmail.com on December 15, 2011 16:19:39 I was just reviewing the changes that you committed for the setting of the proxy. Thanks for accommodating this request.

One final suggestion related to this (or perhaps you have another suggestion I can use instead - ;)). The proxy setting is not available via the Push static methods. I can see the code that you have there for, say, Push.payloads(). Currently, the only way that I can figure to do the work that method does while also setting a proxy is to copy your code into my class and add the line to call setProxy() in the appropriate place.

That's not a huge deal, I can certainly do that. Is that your recommendation? Another option might be to overload or add setting the proxy to the required Push methods.

Thanks again for seeing the value in my original request.

mwkirk commented 11 years ago

From sype...@gmail.com on December 15, 2011 16:43:34 Indeed, upon reflection, the solution isn't perfect yet. Maybe after all that the PushNotificationManager.setProxy method should be un-deprecated and modified to set library-specific properties instead of the JVM-wide ones, as you suggested initially. I'll do the modifications shortly...

Re-opening issue because fix should be improved.

mwkirk commented 11 years ago

From sype...@gmail.com on December 16, 2011 16:36:50 I have just committed r344 which includes a new (and I believe more intuitive) way of configuring proxies. To set a proxy for the entire library, invoke javapns.communication.ProxyManager.setProxy(host, port); A proxy can still be configured for specific AppleServers, or for the entire JVM, but the preferred way would be to set it for the entire library with this method.

Could you let me know if it meets your needs and expectations, so this issue can be closed?

Thank you!

mwkirk commented 11 years ago

From petend...@gmail.com on December 16, 2011 20:56:41 Hi Sylvain,

I just reviewed the changes and I agree that it is much more intuitive. I like all 3 of the options provided (JavaPNS, JVM and server), and the order that you are checking them is, I believe, appropriate. I'm at home today so I won't be able to make the changes to my program and test them until Monday, but at first glance, I like it. :)

Assuming everything runs fine in my test, I'll provide you a final comment on Monday.

Thanks.

mwkirk commented 11 years ago

From petend...@gmail.com on December 20, 2011 15:26:28 I just tested out the changes related to ProxyManager and they are working great and not impacting other processes in the JVM. Thanks again for the quick turnaround.

mwkirk commented 11 years ago

From sype...@gmail.com on December 20, 2011 15:42:20 Awesome! Thank you for the quick feedback!

Closing as fixed.