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

ETag support for WebServer #7709

Merged
merged 13 commits into from
Dec 1, 2020

Conversation

imwhocodes
Copy link
Contributor

This pull request for ETag cache support on static files

This is done by deriving ESP8266WebServerETag from ESP8266WebServer

The hash for Etag is done with the native MD5Builder

This features is very useful when developing web sites on esp8266, only file actually changed during development are downloaded again

Also the existing max-age= can be too broad and is easy to mismanage

Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Very interesting, I see how it could be of use.

Minor little things noted below. Also, can you please put a note in the docs or an example on how to enable this for an end-user? Doesn't need to be a massive writeup, but a line or three in the docs would be of immense use later on.

libraries/ESP8266WebServer/src/ESP8266WebServer-impl.h Outdated Show resolved Hide resolved
libraries/ESP8266WebServer/src/ESP8266WebServer-impl.h Outdated Show resolved Hide resolved
@d-a-v
Copy link
Collaborator

d-a-v commented Nov 16, 2020

Nice, it allows to reduce network activity !
I have a number of questions:

Is this a feature that is only useful while developing web sites, or is it also useful in production ?
If so, should it be transparently enabled ?

- using ESP8266WebServer = esp8266webserver::ESP8266WebServerTemplate<WiFiServer>;
+ using ESP8266WebServer = esp8266webserver::ESP8266WebServerETagTemplate<WiFiServer>;

or with an option (arduino menu and / or global define) ?

using ESP8266WebServerNoETag = esp8266webserver::ESP8266WebServerTemplate<WiFiServer>;
using ESP8266WebServerETag = esp8266webserver::ESP8266WebServerETagTemplate<WiFiServer>;
#if ETAG_ENABLED
using ESP8266WebServer = ESP8266WebServerETag;
#else
using ESP8266WebServer = ESP8266WebServerNoETag;
#endif

or on sketch request (as proposed by your PR) ?

using ESP8266WebServer = esp8266webserver::ESP8266WebServerTemplate<WiFiServer>;
using ESP8266WebServerETag = esp8266webserver::ESP8266WebServerETagTemplate<WiFiServer>;

Does it slow down web requets on big files since it is calculated on every request ?
Did you think of caching the results ?

@imwhocodes
Copy link
Contributor Author

imwhocodes commented Nov 16, 2020

Thanks for the appreciation

Nice, it allows to reduce network activity !
I have a number of questions:

Is this a feature that is only useful while developing web sites, or is it also useful in production ?

IMHO it depends on the situation:

  • Etag cache control tag always do a round trip for the requested page, but the response is only a status code (304) if the file is unchanged
  • max-age cache control tag most of the time do not even ask for the page, but too short of a time and you don't cache, too long and you can miss updates (or the browser can ignore the timeout and ask the page again anyway)

But the two cache control methods could be used together for maximum caching (in the next implementation?)

If so, should it be transparently enabled ?

- using ESP8266WebServer = esp8266webserver::ESP8266WebServerTemplate<WiFiServer>;
+ using ESP8266WebServer = esp8266webserver::ESP8266WebServerETagTemplate<WiFiServer>;

or with an option (arduino menu and / or global define) ?

using ESP8266WebServerNoETag = esp8266webserver::ESP8266WebServerTemplate<WiFiServer>;
using ESP8266WebServerETag = esp8266webserver::ESP8266WebServerETagTemplate<WiFiServer>;
#if ETAG_ENABLED
using ESP8266WebServer = ESP8266WebServerETag;
#else
using ESP8266WebServer = ESP8266WebServerNoETag;
#endif

or on sketch request (as proposed by your PR) ?

using ESP8266WebServer = esp8266webserver::ESP8266WebServerTemplate<WiFiServer>;
using ESP8266WebServerETag = esp8266webserver::ESP8266WebServerETagTemplate<WiFiServer>;

This is a good question...
Implementing it transparently would be very cool but I didn't because it would be a breaking change:

  • It increase the memory footprint (a String of size 24) for every statically served page (a locally stored Etag string plus another possible Etag parsed from browser request header during handling)
  • For now it don't support the dynamic append of index.htm or index.html if requested page is a folder (being Etag calculated only once)

I opted for the on sketch request for backward compatibility and for keeping old code excepted behaviour, but being this PR on the 3.x branch maybe can this different behaviour acceptable?

Does it slow down web requets on big files since it is calculated on every request ?
Did you think of caching the results ?

Actually as implemented right now the hash in only computed once when you call ESP8266WebServerETag::serveStaticETag, then StaticRequestETagHandler::handle only check if the browser request Etag is the same as the one before computed

An option for a transparent implementation could be to compute the ETag every time a static page is requested, it would:

  • keep the excepted behaviour of dynamic index.html if page requested is folder of ESP8266WebServer
  • smaller memory footprint than ESP8266WebServerEtag
  • Etag and max-age at the same time
  • but Hashing of the file every time

In this case/implementation a (arduino menu and / or global define) would be ideal

@imwhocodes
Copy link
Contributor Author

With this last commit now there is Transparent ETag support with backward compatibility by splitting and specialising StaticRequestHandler for the StaticDirectoryRequestHandler and StaticFileRequestHandler case

Inside StaticDirectoryRequestHandler the code is almost the same as the one for the original StaticRequestHandler but the _isFile propriety and logic is missing being implicit of the polymorphism

On the StaticFileRequestHandler the hash is compute once at creation and stored as byte array, only during handle it is encoded as base64 to minimize overhaul memory usage

The choice if to use the Directory or File polymorphism is done during the call to esp8266webserver::serveStatic based on the existence of the argument given fs path

(I will add examples and documentation in a few days)

Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Looks good to me now, thanks!

Copy link
Collaborator

@d-a-v d-a-v left a comment

Choose a reason for hiding this comment

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

Thanks!
Approving without doc nor example because the added feature is configuration-less and transparent for the user.

@d-a-v d-a-v merged commit 7a36874 into esp8266:master Dec 1, 2020
@mathertel
Copy link
Contributor

I have added some improvements in #8227
to make the solution more configurable and working with directories.

@d-a-v || @earlephilhower: This can one be closed.

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.

None yet

4 participants