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

Improvements to the existing ETag implementation #8227

Merged
merged 13 commits into from
Sep 29, 2021
Merged

Conversation

mathertel
Copy link
Contributor

Some improvements to the existing ETag implementation.

  • ETAG header support can be switched on and off on demand (was always on before) using server.enableETag(true)
  • also works with serveStatic when files from a whole directory are served.
  • ETag generator function can be injected. (e.g. to recalc the eTag after updating files)

@mathertel
Copy link
Contributor Author

Some doku:

about ETags

ETags are "hints" delivered with a web response in the ETag Header to the client.
This hint is added to further requests to the same resource in the ."If-None-Match" Header.
Hereby the server can check if the client should still use the version in the cache or needs an update. If the version is unchanged a http return code 304 is responded to the client and the network package is a lot smaller.

In the cases when the same clients will request for the same resources the server-side checksum check is a lot more effective than transferring the bytes over the network.
As this is not always true there is an extended option to inject a function that behavior can be customized and tailored to the specific requirements.

enabling ETag support

To enable this in the embedded web server the enableETag() can be used.
(next to enableCORS)

In the simplest version just call enableETag(true) to enable the internal ETag generation that calcs the hint using a md5 checksum in base64 encoded form. This is an simple approach that adds some time for calculation on every request but avoids network traffic.

enabling ETag support customization

The enableETag() function has a second optional parameter to inject a function for ETag calculation of files.

(not yet tested code):
The function enables eTags for all files except *.txt files by reusing the existing etag calculation function:

server->enableETag(true, [](FS &fs, const String path) -> String {
  String eTag;
  if (!path.endsWith(".txt")) {
    eTag = esp8266webserver::calcETag(fs, path);
  }
  return (eTag);
});

I use the function in my project with a counter as a dummy ETag that gets incremented whenever a file changes. This depends on upload and delete functionality of the web server - so this is more complex than just to explain here in text.
I can also add an example using the "last change date" of the filesystem to create ETags.

@d-a-v
Copy link
Collaborator

d-a-v commented Jul 21, 2021

I can also add an example using the "last change date" of the filesystem to create ETags.

A verbose example is always nice to help understand / quickly test.

ETAG header support can be switched on and off on demand (was always on before) using server.enableETag(true)

It was always enabled and will be disable by default. This is a breaking change. Did you do that for coherency with CORS ?
Is it a bad idea to leave it enabled by default ?

@mathertel
Copy link
Contributor Author

mathertel commented Jul 21, 2021

Using ETAG or not depends of the use-case you implement.
It was OFF before version 3.0.0 and ON on single files only without mentioning it (also breaking change ?).
It was creating the ETag value while setting up the server and was never changed even when the file was updated. That behavior "catched" my attention to the implementation...
I think an option is the right way to go - the default can be ON or OFF I don't care.
Regarding the function nearby enableCORS is only that users will find the options easier when they are near to each other on the same object.

The implementation works in my project https://homeding.github.io/ so it's finished but an example I think will help.

@d-a-v
Copy link
Collaborator

d-a-v commented Jul 21, 2021

and ON on single files only without mentioning it (also breaking change ?).

The real question is: can it be always enabled without side effects with or without clients using ETag ?
To avoid changing default behavior on files (enabled), why not leaving the default to enabled (files&dirs) ?

The implementation works in my project https://homeding.github.io/ so it's finished but an example I think will help.

About the example, I think it will be nice to have one (there's none using ::serveStatic()).

@d-a-v
Copy link
Collaborator

d-a-v commented Jul 23, 2021

Thanks a lot for the example !

Maybe too late, for style, there is a script tests/restyle.sh

@mathertel
Copy link
Contributor Author

:) I am using clang-format as I use vscode. Indent of class access modifiers is not supported there (properly)

@mathertel
Copy link
Contributor Author

It's final, Can be merged.

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.

Look goods !
Thanks for the extensive comments in the example

@d-a-v d-a-v added the alpha included in alpha release label Sep 23, 2021
@1e1
Copy link

1e1 commented Sep 28, 2021

Dear commiters,
An ETag is not necessary a MD5 hash and it has no security aspect.
Can you benchmark (time and memory) with other algos like crc32, sha1, etc?
https://github.com/esp8266/Arduino/pull/8227/files#diff-71e665c8b4e1782630282f4b7aea8b43c239861b73edae4abb665233b28a905fR20-R24

@mcspr
Copy link
Collaborator

mcspr commented Sep 29, 2021

Dear commiters, An ETag is not necessary a MD5 hash and it has no security aspect. Can you benchmark (time and memory) with other algos like crc32, sha1, etc? https://github.com/esp8266/Arduino/pull/8227/files#diff-71e665c8b4e1782630282f4b7aea8b43c239861b73edae4abb665233b28a905fR20-R24

True. Looking for at least one example, here's the common config from apache:
http:https://httpd.apache.org/docs/current/mod/core.html#FileETag

Which includes both 'lightweight' content-length + modification-time (which should be available via through fs api, if one wants to implement something like it) and a more 'heavyweight' digest over the file contents. Plus, some more related to file info

But

ETag generator function can be injected

I also wonder if it's better to inject something simler as the default, and provide an example implementing the 'digest' with various hash functions and inject them with enableETag()

@d-a-v
Copy link
Collaborator

d-a-v commented Sep 29, 2021

I also wonder if it's better to inject something simpler as the default, and provide an example implementing the 'digest' with various hash functions and inject them with enableETag()

Do we have something simpler/faster than calculating MD5 ? Is it that slow ?

I suggest to merge this PR as is (reason: let's move forward!), and someone can update defaults, examples, benchmark with subsequent PRs.
Changing default won't be a breaking change anyway because resulting data and the way they have been obtained are opaque to the outside world.

@mcspr
Copy link
Collaborator

mcspr commented Sep 29, 2021

Do we have something simpler/faster than calculating MD5 ? Is it that slow ?

^
just use the %08X-%08X with mtime and size (aka File::getLastWrite() and File::size())
as other digest function(s)still wants to read the actual file contents

I suggest to merge this PR as is (reason: let's move forward!), and someone can update defaults, examples, benchmark with subsequent PRs.
Changing default won't be a breaking change anyway because resulting data and the way they have been obtained are opaque to the outside world.

Also true.

@d-a-v d-a-v merged commit 3f4bcbe into esp8266:master Sep 29, 2021
@mathertel
Copy link
Contributor Author

Do we have something simpler/faster than calculating MD5 ? Is it that slow ?

The enableETag function has an optional parameter to pass a function. MD5 is only the (slow) default .
Example from my projects:

server->enableETag(true, [this](FS &, const String &path) -> String {
  String eTag;
  File f = fs.open(path, "r");
  eTag = f.getLastWrite()
  f.close();
  return (eTag);
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alpha included in alpha release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants