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

Answer https without connecting upstream. #282

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ganskef
Copy link
Collaborator

@ganskef ganskef commented Mar 16, 2016

This enables a caching proxy for offline use, which is supported with HTTP at the moment for HTTPS too.

It is needed to suppress the handshake to upstream without a connection. Since it's necessary to disable resolving of IP addresses a HostResolver answering unresolved addresses is needed, too. This is used to detect the offline mode in LittleProxy Man-In-The-Middle handling.

@jekh
Copy link
Collaborator

jekh commented Mar 17, 2016

While offline it is neccessary to disable resolving of IP addresses. You must have a HostResolver answering unresolved addresses. This is used to detect the offline mode in LittleProxy Man-In-The-Middle handling too.

This is what I’m skeptical of: I don’t think InetSocketAddress.isUnresolved() should be used to say “don’t connect to the upstream server”. The HostResolver’s job isn’t to determine whether to connect to an upstream server; its job is to resolve hosts. I think the “flag” indicating that the CONNECT should be skipped needs to be explicit, and I don’t think it’s related to DNS resolution. Ideally, the decision of whether to connect to the upstream server would be made before the HostResolver is ever invoked.

I personally would love to see a refactoring of ConnectionFlow, but I'm not sure what that would look like. Short of a refactoring, if there's a way to more explicitly indicate "do not connect", that would probably be fine.

This enables a caching proxy for offline use, which is supported with
HTTP at the moment for HTTPS too. It's needed to suppress the handshake
to upstream without a connection. Since it's necessary to disable
resolving of IP addresses a HostResolver answering unresolved addresses
is needed, too. This is used to detect the offline mode in LittleProxy
Man-In-The-Middle handling.
@ganskef
Copy link
Collaborator Author

ganskef commented May 22, 2016

I have squashed the PR into one commit based on the current master. Since the host name is available now, I've updated the initial comment. These are the modifications I like to pull.

I personally would not like to add another option to the connection flow. Checking the unresolved address to indicate the offline state works well and seems to be clear to me. There is a test (setUp) to show what happens. Otherwise I need them both.

@stabbylambda
Copy link

I just ran into this issue on a build slave that doesn't have access to the internet. I'm wondering if there's a resolution here?

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