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

Path toward testing with a released version of hypercorn? #3334

Open
musicinmybrain opened this issue Jan 31, 2024 · 3 comments
Open

Path toward testing with a released version of hypercorn? #3334

musicinmybrain opened this issue Jan 31, 2024 · 3 comments

Comments

@musicinmybrain
Copy link

Context

Since 2.2.0, the urllib3 test suite relies heavily on hypercorn, but it’s a special patched hypercorn:

# https://github.com/pgjones/hypercorn/issues/62
# https://github.com/pgjones/hypercorn/issues/168
# https://github.com/pgjones/hypercorn/issues/169
hypercorn @ git+https://github.com/urllib3/hypercorn@urllib3-changes

I followed the links, but what I can’t tell is, is this something expected to continue indefinitely? Or is there a reasonable path toward using a released version of Hypercorn in the future?

As a Fedora Linux packager, it’s straightforward for me to create a new python-hypercorn package in order to run urllib3’s tests during the RPM build, but it’s fairly impractical for me to use a special forked copy of Hypercorn that isn’t packaged.

Alternatives

I can stop running the tests, which greatly increases the risk of undetected regressions.

I might be able to bundle a copy of the “special Hypercorn” to use for running the tests, but this is a very unusual approach that would be messy and tedious to maintain. I plan to experiment with this.

Duplicate

I don’t think this has been requested before.

Contribution

I would be willing to submit PR’s to this project or to Hypercorn, although I don’t have a deep understanding of either project’s internals and I am not likely to study them enough to reach that point.

@pquentin
Copy link
Member

pquentin commented Feb 1, 2024

Hey @musicinmybrain, thanks for this. And as a Fedora user, thanks for your packaging work! We switched to Hypercorn because it supports HTTP/2 (unlike Tornado), so switching back isn't an option. Sorry that this causes you more work. :( We definitely want to merge our changes upstream, but had little success so far. The urllib3-changes branch contains three important changes for us:

  1. Support for the ASGI TLS extension. The current support is partial because it's what urllib3 needed, but I'm happy to contribute complete support. It's computationally expensive, so probably needs to be behind a a configuration option.
  2. Access to the underlying transport, which I also implemented as an ASGI extension, though no spec currently exists for this. It was the only way to implement CONNECT, which we need to test our proxy support. It seems reasonable to include to me, and the leading underscore makes it clear that no guarantee exists.
  3. Fail loudly in case of h11/h2 errors. The default behavior is to drop errors silently, which makes debugging extremely difficult. I'm happy to put this behind an option too.

@pgjones What do you think about the above? How can we best help you add those features? We can direct some of our HTTP/2 funding to Hypercorn work on those three issues.

@musicinmybrain
Copy link
Author

Thank you for writing a full and clear explanation in one place!

I have a working proof of concept for introducing python-hypercorn, python-quart, and python-quart-trio packages to Fedora, and then using PYTHONPATH manipulation to interpose a bundled copy of the forked Hypercorn while running the tests in the python-urllib3 package build. Subject to further review and input from other Fedora packagers, I think that will be the path forward for now.

@pgjones
Copy link

pgjones commented Feb 4, 2024

  1. I'm not sure the ASGI TLS extension can be implemented in full - last time I looked a lot of the aspects weren't possible. I think the ASGI extension specification needs to be rewritten to account for this.
  2. & 3. I need to think about these, I had in general not wanted to deviate from the ASGI spec with custom features. I'm more open to 3, as I think the ASGI scope should be simple str/bytes only.

My general issue is time, of which I have very little. PRs are welcome though.

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

No branches or pull requests

3 participants