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

RFC 42: QUIC server in WPT #42

Merged
merged 5 commits into from
Mar 19, 2020
Merged

RFC 42: QUIC server in WPT #42

merged 5 commits into from
Mar 19, 2020

Conversation

Hexcles
Copy link
Member

@Hexcles Hexcles commented Feb 5, 2020

@jgraham
Copy link
Contributor

jgraham commented Feb 6, 2020

@annevk Assuming I look at the implmentation details, is there someone from Mozilla who should give feedback on whether this meets any requirements from our side?

rfcs/quic.md Outdated
similar to `wptserve`. The APIs will likely be different. To more easily
distinguish the two, we add a filename flag to QUIC custom handlers
(`.quic.py`). These handlers will be ignored by `wptserve`. Tests that require
the QUIC server need to be marked with a meta tag (tentatively `<meta
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a filename flag because it isn't about loading the tests themselves over quic, but about being able to filter out quic-requiring tests where the server isn't available, correct? So we might hope to remove this in the future. Is there some way to enforce this; it seems like it's easy for a test not to add the meta element and still work correctly in configurations where quic is enabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't a filename flag because it isn't about loading the tests themselves over quic, but about being able to filter out quic-requiring tests where the server isn't available, correct?

Correct. I thought about using .quic.html, similar to .h2, but realized that they are in fact different. The test themselves should not be loaded via QUIC.

So we might hope to remove this in the future.

Probably, when all major vendors have WebTransport implementations and we run these tests by default.

Is there some way to enforce this; it seems like it's easy for a test not to add the meta element and still work correctly in configurations where quic is enabled?

Great point! I don't have good ideas right now. Any thoughts?

### Notable dependencies

* [sys] Python 3 (>=3.6)
* [py] aioquic
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we happy with the mainteinance story of this library? How did you decide that it has the features we need to give the low-level protocol access that's required for writing tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

@yutakahirano knows more details here.

* [sys] openssl (>=1.0.1, already required by [HTTP/2](http2.md))
* [py] pylsqpack

All of the Python dependencies above have native extensions, which requires
Copy link
Contributor

Choose a reason for hiding this comment

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

So it seems like updating the version here is going to be an infra-breaking change for vendors because work will be required on the vendor side to update to the new version (e.g. updating the deps file or uploading the wheel to the internal pypi mirror or whatever each vendor does). That seems like a pretty big concern to me. If we did vendor the source, we could have logic like "check if the correct version is available, or pip install, or build from source". Maybe there are other possibilities to address this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm thinking about this from two perspectives:

  • For average human users, it's most likely they are using a platform supported by PyPI wheels, and the dependencies will be installed into _venv using pip automatically. Vendoring the source isn't very useful for them.
  • For browser infras, AFAIK both Firefox and Chromium discourage or even disallow building a native Python extension at run time; these dependencies will have to come from a PyPI-like repo. If that's the case, vendoring the source doesn't seem helpful, either.

I agree that this will incur a maintenance burden on the vendors if they want to run QUIC tests. (I'll acknowledge this in the risks section.) Meanwhile, I don't see vendoring the source code would help the browser vendors if my understanding above is correct -- someone would still need to build and distribute the new version.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think our test environment forbids building a native extension, although it also seems likely that the required libraries aren't available. What would definitely be possible (although not tivial) is adding the build step to the Firefox build and distributing the resulting package to the test machines; we already have various test-only binaries that need to be produced.

What worries me about the proposed setup is that updating the library version is a sync-blocking change for Gecko since we're a) using the tools/ directory directly and b) don't allow PyPI access from test machines for robustness reasons.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true for Chromium, too. I imagine that in the future when we need to update these native dependencies, we will need to create a PR that updates the requirements.txt for venv, and ask each vendor to prepare the packages in their infras, before merging the PR. Native dependencies are difficult to deal with in general; I'm intentionally making some tradeoff here between ongoing maintenance costs and complexity.

Do you have a more concrete alternative proposal? If you think vendoring the source into the codebase is easier for Firefox infra, then I'd be happy to add that as a fallback to wheels, too. I just didn't want to vendor them if it wouldn't be used by anyone.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that adding the source would in principle be better for Mozilla, since we could in theory add it to the build system. In practice I don't know that we'd actually get around to doing that :/ It probably depends on how hard it is vs doing manual updates. But I think I'd like us to do it anyway.

In any case I think the process probably demands an RFC for version updates, since it's a change that affects multiple vendors. That's pretty heavyweight, but this really is the kind of change that can break integrations.

@martinthomson
Copy link

I think that this is a reasonable plan, but I can't comment on the specifics until I have more information about webtransport. In that respect, this is probably well premature.

If you regard the current design being proposed by Google as a fait accompli, this is probably enough, but the extra effort required to build something usable on top of a generic QUIC stack could turn out to be such that this approach becomes unwieldy.

@Hexcles
Copy link
Member Author

Hexcles commented Feb 6, 2020

Thanks for the constructive and insightful feedback!

I want to highlight one point: this has NOT been implemented yet, and the design of the QUIC server itself (internal architecture, handler API, etc.) is intentionally vague because I don't have the details yet and we will probably iterate on the design once we start prototyping -- @yutakahirano and the team will work on this.

The key part of this RFC is the integration between the QUIC server and the rest of WPT infra, which should not need to change regardless of the detailed design of the QUIC server itself. This RFC paves the way for prototyping and eventually productionizing the QUIC server within WPT with minimum overhead. We can send out another RFC when we are happy with a detailed server design or implementation.

1. Switch to off-by-default
2. Clarify the scope of the RFC (i.e. not including server APIs)
3. Clarify dependency management and list it as a "risk"
@Hexcles
Copy link
Member Author

Hexcles commented Feb 14, 2020

@jgraham I've updated the RFC:

  1. Switch to off-by-default
  2. Clarify the scope of the RFC (i.e. not including server APIs)
  3. Clarify dependency management and list it as a "risk"

Could you take another look? There are a few open questions now:

  • Deferring the detailed server API design
  • Unable to strictly enforce that a test without the QUIC tag does not need QUIC
  • Dependency management

Do you see any of these as blocking? These are some genuinely good questions to which I don't have perfect answers to. The currently proposed design makes some trade-offs in these areas.

@Hexcles
Copy link
Member Author

Hexcles commented Feb 28, 2020

ping @jgraham

Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

In response to

  • Deferring the detailed server API design

I think this is OK as long as it's off by default. By the time we want broad contributions there needs to be a clear, documented, API for people to write tests against. So that would be before anyone ships QUIC-dependent features by default, for example. I think a commitment to make a server API RFC, and an expected timeline for that in terms of external factors (e.g. implementation status) should be part of this RFC.

  • Unable to strictly enforce that a test without the QUIC tag does not need QUIC

This worries me. We should at least lint for .quic.py in files that aren't marked as requiring quic. But I don't think that there's any provision at the moment for examining dependent files, so a test requiring helper.html that requests helper.quic.py would be harder to catch. I think at the minimum a commitment to add some linting should be part of this RFC.

One other possibility that just occurred to me is to require everything that accesses quic to be in a quic directory and have the server itself reject requests without quic/ in the path. Then you could lint for any occurrence of the string quic outside a quic directory, which would do something to prevent test/foo.html requesting test/quic/helper.html. That's more pain in the long run, but could be relaxed when we turn this on by default.

* [sys] openssl (>=1.0.1, already required by [HTTP/2](http2.md))
* [py] pylsqpack

All of the Python dependencies above have native extensions, which requires
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that adding the source would in principle be better for Mozilla, since we could in theory add it to the build system. In practice I don't know that we'd actually get around to doing that :/ It probably depends on how hard it is vs doing manual updates. But I think I'd like us to do it anyway.

In any case I think the process probably demands an RFC for version updates, since it's a change that affects multiple vendors. That's pretty heavyweight, but this really is the kind of change that can break integrations.

1. Commit to another RFC for server API
2. Add a lint rule
3. Vendor all dependencies
@Hexcles
Copy link
Member Author

Hexcles commented Mar 5, 2020

@jgraham I think I've addressed all the remaining comments. Please take another look at the latest commit. Thanks!

@Hexcles Hexcles requested a review from jgraham March 5, 2020 23:56
@Hexcles
Copy link
Member Author

Hexcles commented Mar 10, 2020

ping @jgraham

Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

Just a minor fix. I think this is OK now, but I foresee this generating future work to improve the checks for metadata accuracy, and to allow upgrades.

@Hexcles Hexcles requested a review from jgraham March 11, 2020 16:43
Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

I think this is OK to land as the basis for experimental support, with the expectation that there will be future RFCs to nail down the API surface and at the point at which we want to switch to on-by-default.

@Hexcles
Copy link
Member Author

Hexcles commented Mar 16, 2020

Heads up: if there's no more comment I'll be merging this in 3 days (which will be a full week after the last comment).

@Hexcles Hexcles merged commit 0635467 into master Mar 19, 2020
@Hexcles Hexcles deleted the quic branch March 19, 2020 18:33
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