drogonframework / drogon

Drogon: A C++14/17/20 based HTTP web application framework running on Linux/macOS/Unix/Windows
MIT License
11.04k stars 1.06k forks source link

Unable to refresh custom 404 page #1979

Open Mis1eader-dev opened 3 months ago

Mis1eader-dev commented 3 months ago

Describe the bug In a real-time system, the 404 page may get updated, and since the custom 404 page is cached in Drogon, we have to call drogon::app().setCustom404Page to refresh its content, however, this doesn't happen. For SPAs this is a significant issue, as the only way to refresh its content is through restarting the server.

To Reproduce Steps to reproduce the behavior:

  1. Create a 404 page
  2. Call drogon::app().setCustom404Page and point it to the 404 file
  3. Open the browser and go somewhere random, note the current page
  4. In the backend edit the 404 page contents and save the file
  5. Refresh the browser, you'll still see the old 404 page
  6. Now create an API endpoint that calls drogon::app().setCustom404Page and returns 200 OK
  7. Call the API
  8. Still shows the old 404 page

Expected behavior Either the 404 shouldn't be cached, or we should be allowed to refresh it.

Desktop (please complete the following information):

hwc0919 commented 3 months ago

I think we don't need to pursue extreme performance in these cases. An atomic shared pointer is enough. @an-tao

https://github.com/drogonframework/drogon/blob/4cbac301fbebb369dba8935f4f727a1ba5b9c513/lib/src/HttpAppFrameworkImpl.cc#L975-L1000

hwc0919 commented 3 months ago

You could use setCustomErrorHandler() before we handle this problem.

The custom handler will be invoked every time, no internal caching.

PS: To make it into use, do not call setCustom404Page().

Mis1eader-dev commented 3 months ago

Performance-wise will newFileResponse calls in the custom error handler be cached? If every request hitting the error handler ends up making an OS file request repeatedly then that will degrade performance quite a lot for SPAs

Say the custom error handler is set to always

return HttpResponse::newFileResponse(drogon::app().getDocumentRoot() + "/index.html");

And we have 10000 active requests every second to /some/page, does the first request do a file read and the rest will be cached until the file Date Modified changes?

an-tao commented 3 months ago

Performance-wise will newFileResponse calls in the custom error handler be cached? If every request hitting the error handler ends up making an OS file request repeatedly then that will degrade performance quite a lot for SPAs

Say the custom error handler is set to always

return HttpResponse::newFileResponse(drogon::app().getDocumentRoot() + "/index.html");

And we have 10000 active requests every second to /some/page, does the first request do a file read and the rest will be cached until the file Date Modified changes?

You could use the setExpiredTime mehtod of HttpResponse to achieve that.

Mis1eader-dev commented 3 months ago

Thank you @an-tao and @hwc0919, using a combination of resp->setExpiredTime(drogon::app().staticFilesCacheTime()) and drogon::app().setCustomErrorHandler(...) seems to achieve the desired result

I don't know whether this issue can be closed, it needs proper documentation to let users know drogon::app().setCustom404Page(...) is called only once, and will stay cached until the application exits

Mis1eader-dev commented 3 months ago

This does not seem to be the way to do this:

static void postConfigure()
{
    /// vue.js integration (SPA)
    auto resp = HttpResponse::newFileResponse(
        drogon::app().getDocumentRoot() + "/index.html",
        "",
        drogon::CT_TEXT_HTML,
        "text/html"
    );
    resp->setExpiredTime(drogon::app().staticFilesCacheTime());
    drogon::app()
        .setCustomErrorHandler(
            [
                resp = std::move(resp)
            ](HttpStatusCode, const HttpRequestPtr&)
            {
                return resp;
            }
        );
}

int main()
{
    drogon::app()
        .loadConfigFile("./config.json");

    postConfigure();

    drogon::app()
        .run();
}

The response will always be the old cached index.html page.

Will it be cached if the

auto resp = HttpResponse::newFileResponse(...

is moved into the custom error handler? I can't find any cache map lookup mechanism for file responses in the framework to safely do this without worrying about it hitting the OS file lookups every time

an-tao commented 3 months ago

You should move the newFileResponse code to the handler and cache the old response by yourself.

Mis1eader-dev commented 3 months ago

I found drogon::StaticFileRouter::sendStaticFileResponse(...) but it is not publicly exposed to be used outside of the framework, and it seems to take in a response callback, which the custom error handler doesn't have.

The only way without reimplementing the static file router would be to create individual pages with HttpSimpleControllers and have each one call drogon::StaticFileRouter::sendStaticFileResponse(...) with the index.html file.

Seems we have to use an atomic drogon::HttpResponsePtr and have the endpoint that gets notified of the page changes assign a new drogon::HttpResponsePtr to the atomic response

Mis1eader-dev commented 3 months ago

For other SPA developers, this is the snippet that may be of help to you

static std::atomic<HttpResponsePtr> indexHtml;

static void updateIndexHtml()
{
    ::indexHtml = HttpResponse::newFileResponse(
        drogon::app().getDocumentRoot() + "/index.html",
        "",
        drogon::CT_TEXT_HTML,
        "text/html"
    );
}

static void postConfigure()
{
    /// vue.js integration (SPA)
    updateIndexHtml();
    drogon::app()
        .setCustomErrorHandler(
            [](HttpStatusCode, const HttpRequestPtr&)
            {
                return ::indexHtml.load();
            }
        )
        /// WebSocket notification of frontend change
        .registerHandler(
            "/api/_/dist", // <- can be anything you like
            [](const HttpRequestPtr &req,
                std::function<void (const HttpResponsePtr&)>&& callback)
            {
                if(!req->peerAddr().isLoopbackIp())
                {
                    callback(HttpResponse::newNotFoundResponse(std::move(req)));
                    return;
                }

                updateIndexHtml();

                callback(HttpResponse::newHttpResponse(drogon::k200OK, drogon::CT_NONE));

                // TODO: Notify the frontend clients to refresh their pages
            }
        )
        ;
}

int main()
{
    drogon::app()
        .loadConfigFile("./config.json")
        ;

    postConfigure();

    drogon::app()
        .run()
        ;
}

You will need some client code somewhere to trigger the /api/_/dist endpoint, a simple curl will refresh the cache:

curl -d '' http://localhost:8080/api/_/dist

If you need to trigger it remotely, you can remove the check

if(!req->peerAddr().isLoopbackIp())
{
    callback(HttpResponse::newNotFoundResponse(std::move(req)));
    return;
}

However, beware of the security risk of removing that check, as anybody could hit that API endpoint and consume your server's resources from constant reloading of the index.html file

hwc0919 commented 3 months ago

You can use an timer to check the md5 change or edit time of the file.

Mis1eader-dev commented 3 months ago

Thank you for the suggestion, in my use case there is a custom CI/CD app made with Drogon that gets triggered by GitHub webhooks, which performs a git pull on the document_root directory and makes a localhost request to the actual server to notify its clients to refresh their pages.

So performance-wise this atomic variable is enough for this application.

If other developers needed automatic refresh, they have to go the MD5 route