Skip to content
This repository was archived by the owner on Jan 21, 2025. It is now read-only.

rework INM/IMS handling for AsyncStaticWebHandler - #190

Merged
mathieucarbou merged 1 commit into
mainfrom
ims
Jan 9, 2025
Merged

rework INM/IMS handling for AsyncStaticWebHandler#190
mathieucarbou merged 1 commit into
mainfrom
ims

Conversation

@vortigont

Copy link
Copy Markdown
Collaborator

Have faced some problems where I received wrong 304 replies for static files.
The issue was that if some file generated and written on filesystem before WiFi connects and aqures proper time file's timestamp was about +2 +3 seconds UTC and that could lead to improper 304 replies to the clients that was previously had this file.
So I've added some fixes and improvements

  • IMS template must contain GMT timezone, not local - "%a, %d %b %Y %H:%M:%S GMT"
    server used to reply with local timezone specifier but used GMT time.

  • create etag header based on timestamp + filesize, it adds a bit of entropy for files which are created soon after boot

  • INM header handling should have precedence over IMS when checking if content is not modified and since etag already includes timestamp further check could be skipped

@mathieucarbou

Copy link
Copy Markdown
Owner

Good catch!

Comment thread src/WebHandlers.cpp Outdated
Comment thread src/WebHandlers.cpp Outdated
Comment thread src/WebHandlers.cpp Outdated
Comment thread src/WebHandlers.cpp Outdated
Comment thread src/WebHandlers.cpp Outdated
Comment thread src/WebHandlers.cpp Outdated
Comment thread src/WebHandlers.cpp Outdated

@mathieucarbou mathieucarbou left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

a few comments ;-)

@vortigont

Copy link
Copy Markdown
Collaborator Author

a few comments ;-)

Nice! Thanks!

IMS template must contain GMT timezone, not local - "%a, %d %b %Y %H:%M:%S GMT"

create etag based on timestamp + filesize

INM header handling should have precedence over IMS
@mathieucarbou

Copy link
Copy Markdown
Owner

@vortigont : I added a comment here about the etag: #190 (comment)

There are still issues regarding the http spec compliance, issues which were already existing before anyway - so worth fixing them also in this PR :-)

@mathieucarbou
mathieucarbou merged commit 537fbe8 into main Jan 9, 2025
@mathieucarbou
mathieucarbou deleted the ims branch January 9, 2025 14:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants