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

Use Plack::LWPish rather than LWP::UserAgent #11

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dex4er
Copy link
Contributor

@dex4er dex4er commented Jan 24, 2014

Hello,

It would be great if Plack::App::Proxy doesn't use LWP::UserAgent but Plack::LWPish. Making LWP as optional module will make that only pure Perl modules are required and none of XS modules at all. It will make easier to install this great module on platforms without compiler.

Unfortunately, HTTP::Tiny (via Plack::LWPish) is strictly HTTP 1.1 compatible and doesn't allow to use custom Host header. Fortunately, we can override some of its method and preserve this header. See chansen/p5-http-tiny#34 for explanation.

I've tested the Plack::App::Proxy with this patch on Android and it works really great! It allows to use the mobile as a HTTP proxy server:

dexter@hp-probook-6460b:~$ ssh htc-one-s -p22022
dexter@htc-one-s's password: 
cctools@localhost:/data/data/com.pdaxrom.cctools/root/cctools/home # uname -a
Linux localhost 3.4.61-LIBERTY-SENSE-v4-g291f820-dirty #1 SMP PREEMPT Thu Sep 12 14:36:20 EAT 2013 armv7l GNU/Linux
cctools@localhost:/data/data/com.pdaxrom.cctools/root/cctools/home # twiggy -MPlack::App::Proxy \
>        -e 'enable q{AccessLog}; enable q{Proxy::Connect}; \
>            enable q{Proxy::AddVia}; enable q{Proxy::Requests}; \
>            Plack::App::Proxy->new->to_app' 
192.168.1.74 - - [24/Jan/2014:21:38:31 +0000] "HEAD http://www.cpan.org/ HTTP/1.0" 200 - "-" "-"
dexter@hp-probook-6460b:~$ telnet htc-one-s 5000
Trying 192.168.1.84...
Connected to htc-one-s.
Escape character is '^]'.
HEAD http://www.cpan.org/ HTTP/1.0

HTTP/1.0 200 OK
Date: Fri, 24 Jan 2014 21:31:59 GMT
Via: 1.1 varnish
Age: 114
ETag: "76ca00-1f76-4f0bdb9a62040"
Server: Apache/2.2.3 (CentOS)
Vary: Accept-Encoding
Content-Type: text/html; charset=UTF-8
Last-Modified: Fri, 24 Jan 2014 21:06:01 GMT
X-Cache: HIT
X-Cache-Hits: 7
X-Served-By: eu3.develooper.com
X-Varnish: 1963777543 1963777251

Connection closed by foreign host.

@leedo
Copy link
Owner

leedo commented Jan 25, 2014

This looks good to me.

Existing users of the LWP proxy backend will be unaffected, which is good. (Initially I thought this was also updating that backend to use Plack::LWPish).

Curious if @miyagawa or @hiratara have any thoughts. If not I'll merge this soon!

@miyagawa
Copy link
Collaborator

Looks okay to me, but if the change was intended to remove LWP from the distribution's hard requirements because of the tests, you could condition out the test with HAS_LWP, and not run it at all. The reason you can run it on Android is that the default backend is AnyEvent::HTTP and it is pure perl, which has been always the case.

Running the test using a different backend depending on which module users have makes the test behavior a bit unpredictable.

Like @leedo said, it does make sense to write a new Plack::LWPish backend that actually runs the proxy using the pure perl module. but since AnyEvent::HTTP is already pure perl and i guess HTTP::Tiny doesn't allow that level of custom hooks, it might not be worth.

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.

3 participants