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

Blue/minimal http proxy #656

Merged
merged 17 commits into from
Sep 22, 2021
Merged

Blue/minimal http proxy #656

merged 17 commits into from
Sep 22, 2021

Conversation

shadeofblue
Copy link
Contributor

@shadeofblue shadeofblue commented Sep 15, 2021

closes #652

@shadeofblue shadeofblue marked this pull request as ready for review September 17, 2021 09:32
@shadeofblue shadeofblue requested review from a team, kmazurek and johny-b September 17, 2021 09:32
Copy link
Contributor

@johny-b johny-b left a comment

Choose a reason for hiding this comment

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

I really like the logic of this example, but I think the implementation could be improved quite a lot.

The general remark is that I would move as much of the instance-only logic as possible to HttpService class.

examples/http-proxy/Dockerfile Outdated Show resolved Hide resolved
examples/http-proxy/html/index.html Outdated Show resolved Hide resolved
examples/http-proxy/http_proxy.py Show resolved Hide resolved
examples/http-proxy/http_proxy.py Outdated Show resolved Hide resolved
examples/http-proxy/http_proxy.py Outdated Show resolved Hide resolved
examples/http-proxy/http_proxy.py Show resolved Hide resolved
examples/http-proxy/http_proxy.py Outdated Show resolved Hide resolved
examples/http-proxy/http_proxy.py Show resolved Hide resolved
examples/http-proxy/http_proxy.py Outdated Show resolved Hide resolved
print(instances())
try:
await asyncio.sleep(10)
except (KeyboardInterrupt, asyncio.CancelledError):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we capture CancelledError here?
I'm not saying it's wrong, I just don't understand, and this is an example - so things should be understandable : )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it's because the KeyboardInterrupt can happen in some other part of async code and thus, the KeyboardInterrupt won't happen in the asyncio.sleep(10) but in some other place and that routine will have been cancelled

Copy link
Contributor

Choose a reason for hiding this comment

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

@shadeofblue
OK, that makes sense.

We have a comment "wait until Ctrl+C". I think it would be nice to add the same explanation to the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm.... that's also present in all the other examples, I'm unsure if we want to pollute this one with this particular explanation

@shadeofblue shadeofblue requested a review from johny-b September 21, 2021 14:34
Copy link
Contributor

@johny-b johny-b left a comment

Choose a reason for hiding this comment

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

image

@shadeofblue shadeofblue merged commit fadebbc into master Sep 22, 2021
@shadeofblue shadeofblue deleted the blue/minimal-http-proxy branch September 22, 2021 08:15
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.

a simple VPN usage example that implements an http proxy
2 participants