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

LiteSpeed support #444

Closed
wants to merge 67 commits into from
Closed

LiteSpeed support #444

wants to merge 67 commits into from

Conversation

Toflar
Copy link
Contributor

@Toflar Toflar commented Nov 21, 2018

WIP for Open Source Litespeed (OLS) and possibly the commercial Litespeed Web Server (LSWS).

Cache tagging is only implemented in OLS but not at all in LSWS. The changes worked with OLS but the latest OLS version seems to have done BC breaks / regressions with cache tagging.

The original author does not continue on this - if you need this, you will need to wrap up this merge request or pay someone to do it...


This is a possible implementation for LSWS (https://www.litespeedtech.com/products/litespeed-web-server) support.

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

cool, thanks for this pull request!

interesting approach. is lightspeed supposed to be used in multi-node setup? either way we should discuss that a bit in the documentation to avoid confusion.

the header file is a good solution, but seems against the philosophy of lightspeed. it should actually only be needed if we invalidate from a non-web context. when there is a web request that triggers the things that trigger invalidation, we would ideally set the headers on the web response.

could we support this pattern here? LiteSpeed client could have an additional method to update a PSR-7 response with invalidation for that use case. we would still call flush, in case either there was no web response or when invalidations happen after the response has been sent. for the symfony bundle, we can extend the client to also be able to update a symfony response.

its a bit borderline to push that job into the client, but i think its in the domain of the lightspeed client. we could maybe generalize this concept to define an interface for this pattern to have our listeners check for an interface instead of litespeed specifically.

doc/litespeed-configuration.rst Outdated Show resolved Hide resolved
doc/litespeed-configuration.rst Outdated Show resolved Hide resolved
doc/litespeed-configuration.rst Outdated Show resolved Hide resolved
src/ProxyClient/LiteSpeed.php Outdated Show resolved Hide resolved
src/ProxyClient/LiteSpeed.php Outdated Show resolved Hide resolved
doc/proxy-clients.rst Outdated Show resolved Hide resolved
@Toflar
Copy link
Contributor Author

Toflar commented Nov 21, 2018

I think you got something about LiteSpeed wrong in general. LiteSpeed is a drop-in-replacement for Apache. It's not an Apache module but it supports .htaccess configuration. It's web server and proxy all in one and it's super fast (it's actually a pity many people don't know about it).
Do you want to review your comments again? It should answer a few things like multi-node setup questions and is this an apache module? 😄

@dbu
Copy link
Contributor

dbu commented Nov 21, 2018

I think you got something about LiteSpeed wrong in general. LiteSpeed is a drop-in-replacement for Apache.

maybe we should mention that in the lead then ;-)

multi-node setup is still very much a question though. apache can act as a reverse proxy. if you'd use litespeed that way, you would need to make sure the request to selve goes to the correct backend, or your generated header file is not there. if you use multiple litespeed behind a loadbalancer, you'd need to make sure that each of them gets the request (that they are all in the list of servers) so they see the invalidation resposne. and also that the file is generated on each instance.

@Toflar
Copy link
Contributor Author

Toflar commented Nov 21, 2018

I think I'd rather mention that multi-node stuff is not a covered use-case at all. I won't work on support for that because I won't need it.

@dbu
Copy link
Contributor

dbu commented Nov 21, 2018

multi-node not covered

i think thats good. lets state that explicitly in the documentation, and mention that if you need more than one server with your app, varnish is a good solution for the caching reverse proxy, and its fully supported already.

@litespeedtech
Copy link

Hi, dbu,

This is Lauren from LiteSpeedTech. We also have WebADC product, which follows the same headers. So this solution is generic, if it works with LiteSpeed Web Server, it will work with WebADC. WebADC is a full featured Load balancer with LSCache, ESI, HTTP/2, QUIC support built in. We already have many clients used it production environment, especially for big Magento stores.

Thanks for great effort Toflar put in. We can together improve this. This is something we always wanted to do, but haven't got chance.

We have a Free Starter license (allow for 1 Domain, 2GB memory), this allows more devs can give a try in his local environment. Hopefully we can get more feedback.

Thanks,
Lauren

@Toflar
Copy link
Contributor Author

Toflar commented Nov 21, 2018

the header file is a good solution, but seems against the philosophy of lightspeed. it should actually only be needed if we invalidate from a non-web context. when there is a web request that triggers the things that trigger invalidation, we would ideally set the headers on the web response.

It's the only method that is supported to invalidate cache tags and I actually find it a pretty nice solution. It's really easy to setup and it works across different applications or frameworks. Setting it on the response would again require a framework-specific implementation (e.g. a listener for Symfony).

{
$filename = 'fos_cache_litespeed_purger_';

if (function_exists('random_bytes')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dbu we're not talking crypto-stuff here and we're deleting the file right away so probably this part here is way to defensively coded 😄 Maybe a simple sha1(mt_rand()) is perfectly good enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, we only care about avoiding collisions. even if somebody could guess that url, nothing too bad happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay done in 8e627ab.

src/ProxyClient/LiteSpeed.php Outdated Show resolved Hide resolved
@dbu
Copy link
Contributor

dbu commented Nov 21, 2018

hi @litespeedtech Lauren , thanks for looking at this. is WebADC the commercial licensed version of LiteSpeed? or is it a pure reverse proxy without the capability to run web applications itself, but follows the same instructions as LiteSpeed?

and what do you think of the discussion about hooking on the actual reply to the request for invalidation header, when possible?

@Toflar
Copy link
Contributor Author

Toflar commented Nov 21, 2018

and what do you think of the discussion about hooking on the actual reply to the request for invalidation header, when possible?

It would be better of course. It's faster. But then we need to have access to the response somehow and this is something completely new we haven't used anywhere else before.

@litespeedtech
Copy link

litespeedtech commented Nov 21, 2018

@dbu WebADC is a different product than LSWS, it's a Layer 4/7 load balancer with LSCache. That's for our client who needs a cluster solution, they will need to use WebADC, as caching needs to be at the first layer.
Ideally if it's doable via direct response header, that will be great. That's our current cache plugin does. Here we have a graph that showing the advantage over varnish. To serve same amount of traffic, our internal tests showing litespeed uses about 1/3 of CPU & memory of what varnish consumes.
Another advantage of LiteSpeed ESI implementation, is we supports esi:inline. If one page has many esi:include, we can use inline to do all with one response. This is used in our Prestashop plugin. But this is easier to do at cache plugin layer, not easy to do at framework level without much knowledge of the real app.

src/ProxyClient/LiteSpeed.php Outdated Show resolved Hide resolved

$this->queueRequest('GET', $url, []);

$result = parent::flush();
Copy link
Contributor

Choose a reason for hiding this comment

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

flush might throw an ExceptionCollection. deletion should be in a finally block

src/ProxyClient/LiteSpeed.php Outdated Show resolved Hide resolved
@Toflar
Copy link
Contributor Author

Toflar commented Dec 8, 2018

Failing test is unrelated, maybe you need to retrigger the build. I did not update the docs yet as I want to have your feedback on the implementation first 😄

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

i looked into this and think we are close. apart from nitpicks, i think we need to check about using the correct host in requests to invalidate with the more than one hostname scenario.

any chance that you could add a functional test that installs litespeed on travis-ci? i know its quite an effort, but would make it certain that things actually work and keep working after refactorings, which would be a huge plus.

src/ProxyClient/LiteSpeed.php Outdated Show resolved Hide resolved
src/ProxyClient/LiteSpeed.php Outdated Show resolved Hide resolved
src/ProxyClient/LiteSpeed.php Outdated Show resolved Hide resolved
src/ProxyClient/LiteSpeed.php Outdated Show resolved Hide resolved
src/ProxyClient/LiteSpeed.php Outdated Show resolved Hide resolved
src/ProxyClient/LiteSpeed.php Outdated Show resolved Hide resolved
src/ProxyClient/LiteSpeed.php Show resolved Hide resolved
tests/Unit/ProxyClient/LiteSpeedTest.php Outdated Show resolved Hide resolved
src/ProxyClient/LiteSpeed.php Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

I'll have a look at functional tests but I cannot promise.

src/ProxyClient/LiteSpeed.php Outdated Show resolved Hide resolved
src/ProxyClient/LiteSpeed.php Outdated Show resolved Hide resolved
src/ProxyClient/LiteSpeed.php Outdated Show resolved Hide resolved
src/ProxyClient/LiteSpeed.php Show resolved Hide resolved
src/ProxyClient/LiteSpeed.php Outdated Show resolved Hide resolved
src/ProxyClient/LiteSpeed.php Outdated Show resolved Hide resolved
src/ProxyClient/LiteSpeed.php Outdated Show resolved Hide resolved
src/ProxyClient/LiteSpeed.php Outdated Show resolved Hide resolved
tests/Unit/ProxyClient/LiteSpeedTest.php Outdated Show resolved Hide resolved
@Toflar Toflar force-pushed the litespeed branch 2 times, most recently from ae8e37c to ebd0600 Compare January 22, 2019 16:05
@dbu dbu mentioned this pull request Jun 20, 2019
qsCache 1

# Disable checking for a cached entry if there's a cookie on the request
reqCookieCache 0
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't support user context caching with litespeed, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. User context caching requires some kind of preflight request to the app to find out the context which requires implementation in the proxy.

@dbu
Copy link
Contributor

dbu commented Nov 27, 2019

i notice that this seems mostly done. any chance we can wrap it up and merge it?

@Toflar
Copy link
Contributor Author

Toflar commented Nov 27, 2019

It just works with OLS. Apparently there's no interest to integrate the same logic in LSWS which makes absolutely no sense to me. Looks like LiteSpeed has two different products which do not share any of the code base, not even the functional tests :(

@dbu
Copy link
Contributor

dbu commented Nov 27, 2019

strange. well, lets wrap it up for OLS and add a note that LSWS behaves different and would need more work?

@Toflar
Copy link
Contributor Author

Toflar commented Nov 27, 2019

Yeah, I'll add the docs and write that LSWS currently does not support tag invalidation.

@Toflar
Copy link
Contributor Author

Toflar commented Nov 29, 2019

I really doubt that I want to finish this PR at this point.

First of all, I set out to do this PR because I had customers running behind LSWS and as there seems to be no effort at all in implementing cache tag purging there, there's no benefit for me or my customers. So if I was to finish this, it would be pure goodwill because I cannot use it myself anyway.

And looking at the failing unit tests after updating to the latest stable OLS release, there are random failures again. So I feel like I am the one that is functional testing OLS and that's definitely not the purpose of this PR. I mean I could of course dig into why exactly OLS doesn't behave like it should anymore but then again, I don't use it myself...why would I?

@dbu dbu changed the title Added LiteSpeed support LiteSpeed support Nov 29, 2019
@dbu
Copy link
Contributor

dbu commented Nov 29, 2019

pitty, but fair enough. totally understand you.

i suggest we keep this around for a while still, in case somebody wants to continue working on it. i updated the description and marked the PR as needing a new contributor.

@dbu
Copy link
Contributor

dbu commented Jan 12, 2022

i close this as it is pretty old.

if somebody wants to pick things up, please feel free to do so, i am not against having a proxy client for litespeed in here. note that with all the refactorings that happened since this was started, the proxy client might has to look different than in this PR.

@dbu dbu closed this Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants