-
Notifications
You must be signed in to change notification settings - Fork 59
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
Add Support HTTP Multiplexing #3101
base: develop
Are you sure you want to change the base?
Conversation
Thank you for the contribution!
To achieve this, add a setting to the
Then, access it from the viewer code like this:
|
I doubt that all services will pick up http/2 in one go, so it's probably beneficial to do it on a case by case bassis viewer side, like HttpOptions (or header?).
I doubt that it's accessible from curl's files. P.S. using gSavedSettings.getBOOL() directly for a code like this is suboptimal, it probably should be LLCachedControl inited from gSavedSettings... |
I'm currently working on it, and at least I added gSavedSettings to "indra/llcorehttp/_httplibcurl.cpp", but it didn't build. |
You can also remake mPipelined into a enum with options like no|pipelining|multiplexing. It already loads from gSavedSettings.getBOOL(http_pipelining), so it will just have to become getS32(http_pipelining). |
I don't understand how to use HttpOptions in _httplibcurl.cpp. |
Per-request and per-multi-handle options are kept well-apart in llcorehttp. So It sounds like you want to extend the policy class definition. But maybe describe what libcurl options you want to change and that will shape an answer. There's also the README.Linden file which hasn't been maintained but has useful tutorial information. |
74f1de1
to
beeb609
Compare
It is now possible to switch between CURLPIPE_MULTIPLEX or CURLPIPE_HTTP1 depending on the value registered in httppolicyclass. --- Here's what I think --- |
I think it should work, let me know if you have any problems. |
@@ -281,6 +283,15 @@ void LLAppCoreHttp::init() | |||
LL_INFOS("Init") << "HTTP Pipelining " << (mPipelined ? "enabled" : "disabled") << "!" << LL_ENDL; | |||
} | |||
|
|||
// Global multiplexing setting | |||
static const std::string http_multiplexing("UseHTTP2Multiplexing"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I would have merged "HttpPipelining" and "UseHTTP2Multiplexing" into some kind of 'mode switch' since they are sort of exclusive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One very large problem I have with these http/2 PRs is that this code, while disabled by default, cannot even be tested experimentally anywhere. Linden has a lot of infrastructure changes to make. (https: support, http/2 support, origin server changes, multiple grids, CDN configuration, new certs and cipher negotiation) So this is code that has never been tested and cannot simply be picked up and developed without that work. I approve of the spirit but this may not be the way to start.
Given the amount of infrastructure that must change and the need for testing around the world, this is work that should probably be packaged in a project viewer and not targeted at 'develop'.
I was surprised when these PRs appeared. Is there some particular need driving the interest in HTTP/2? We do have a project outlined but other work must happen first. And now I am curious if there is an important need for http/2 now....
indra/llcorehttp/_httplibcurl.cpp
Outdated
check_curl_multi_setopt(multi_handle, | ||
CURLMOPT_PIPELINING, | ||
1L); | ||
LL_INFOS("HttpLibcurl") << "HTTP Pipelining Enable." << LL_ENDL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this debug logging left in? Should it not use the supplied LOG_* tag values above?
indra/llcorehttp/_httplibcurl.cpp
Outdated
CURLMOPT_PIPELINING, | ||
1L); | ||
LL_INFOS("HttpLibcurl") << "HTTP Pipelining Enable." << LL_ENDL; | ||
if (options.mMultiplexing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nesting conditionals are probably wrong, in practice. If mMultiplexing is on but mPipelining is off, multiplexing ends up disallowed. Probably not intended. One of those areas we'll have to sort out. If the 'mPipelining' concept is changed to concurrency, this is more nearly correct.
@@ -217,6 +217,10 @@ class HttpRequest | |||
/// Per-class only | |||
PO_PIPELINING_DEPTH, | |||
|
|||
/// If it is 1, enable HTTP Multiplexing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More explanation about this API. The 'public' API presented here is not meant to be a facade for libcurl. At the time this was written and also possibly for future purposes, libcurl might be replaced by another library. (For Sansar, we had our own low-level libcurl-like library which could have slotted in.) The 'public' API avoided using libcurl definitions.
Instead, this API is meant to describe how the viewer wants to use HTTP requests and how HTTP traffic should be managed and shaped. The intent is to have this description be as minimal as possible. If a concept isn't used by the viewer, it won't appear here.
Internal APIs, such as _httplibcurl.*, are the translation layer between the public API and whatever is going on below. An adapter layer. There's only one concrete adapter at this point but that is okay.
And so we have the comment above:
When PIPELINING_DEPTH is 2 or more, libcurl performs
That is still correct. The public API describes an abstract usage and a pipelining depth of 2 or more enables pipelining. And so the code above in _httplibcurl is no longer correct.
What should this API be in the http/2 world? Well, we don't know yet. We haven't stood up the infrastructure to support it. But we would probably want to modify llcorehttp differently. PO_PIPELINING_DEPTH might instead be changed to PO_REQUEST_CONCURRENCY covering both the http/1 (pipelining) and http/2 (multiplexing) cases with added explicit enables/disables for pipelining and http/2. However, we may find that we need independent http/2 parameters for the policy classes.
(There's another weak reason to keep existing API definitions as-is: we use a copy of this code in server-side binaries and subtle changes may not be caught. This isn't meant to stop changes and progress in the viewer. But when all other considerations are equal, keeping an API spec has a benefit to us.)
c2b21ca
to
bd9916f
Compare
The reason we decided to introduce the concept of HTTP/2 to the viewer was the end of support for OpenSSL. One of the obstacles to updating OpenSSL is updating Curl. We believe that if HTTP/2 is successfully introduced, one of the obstacles to updating OpenSSL will be removed. During this time, HTTP has been updated to HTTP/3, but the viewer continues to rely on HTTP/1.1... |
The issues pointed out that did not work as intended have been fixed. I now understand that this content is likely to be something that should be put into the project viewer. |
bd9916f
to
fd14211
Compare
Yes, there is quite a bit of work that must happen on our side. For now, I recommend making this a 'draft pull request' so that it isn't lost. It may be useful to start here in the future. I do have an http/2 project outline. I will put this in a better place later. But I can share it here for reference: Milestone 1 - Testbed
Milestone 2 - Infrastructure
Milestone 3 - Internal Viewer
Milestone 4 - Project Viewer
Milestone 5 - Final Pass
|
Enables HTTP/2 Multiplexing.
If this doesn't work, it will use HTTP pipelining.
(Need Help)I want to be able to enable and disable HTTP/2 Multiplexing using the debug options, but where should I change it?maybe solve