fix: settings writes silently lost, and unknown keys panic the device - #48
Open
socquique wants to merge 2 commits into
Open
fix: settings writes silently lost, and unknown keys panic the device#48socquique wants to merge 2 commits into
socquique wants to merge 2 commits into
Conversation
find() throws std::runtime_error for keys that are not in the settings map, and fromJson() runs on the web server task with no try/catch around it, so an uncaught exception panics the device. A browser still holding a settings page from a different firmware version will post keys this build has never heard of - which is enough to reboot the display from the UI. Unknown keys are now logged and skipped. Also release the nvs handle on the validation-failure path, which returned without calling end(). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
JsonSettings shares one Preferences instance between the Arduino loop task and the AsyncTCP task that serves the HTTP handlers, with no lock, and every accessor opens and closes NVS around a single get or put. getMode() runs on every pass through loop(), so the window is wide open: one task's end() closes the handle the other is part way through using. The read then falls back to the compiled default and the write is simply lost, while the request still answers "success" to the browser. Reproduced on an 8 module display: a POST /text returned success, mode read back as 0 (the compiled default for that key) and then reverted to its previous value, and the display never changed mode. A later POST /settings likewise reported success without storing anything; the same request succeeded on the second attempt. After this change, 12 of 12 consecutive write-then-read-back cycles matched. The mutex is recursive because reset() calls fromJson(toJson()) and all three take it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
1. Settings writes are lost, and the request still says "success"
JsonSettingsshares onePreferencesinstance between the Arduino loop task andthe AsyncTCP task that serves the HTTP handlers, with no lock, and every accessor
opens and closes NVS around a single get or put:
loop()callsgetMode()on every pass, so this runs thousands of times a secondwhile the web handlers write from another task. One task's
end()closes thehandle the other is part way through: the read falls back to the compiled default,
the write is dropped, and the browser is told it succeeded.
Observed on an 8 module display: a
POST /textreturnedsuccess,modereadback as
0— the compiled default for that key — then reverted, and the displaynever changed mode. A later
POST /settingsreported success without storinganything and only took on the second attempt.
All accessors now take a recursive mutex; recursive because
reset()callsfromJson(toJson())and all three take it. After the change, 12 of 12 consecutivewrite-then-read-back cycles matched.
2. An unknown settings key panics the device
find()throwsstd::runtime_errorfor keys that are not in the settings map, andfromJson()calls it on every key of the posted JSON with notry/catchanywhere above it. The exception escapes on the web server task and takes the whole
device down.
Any browser holding a settings page from a different firmware version will post
keys this build has never heard of, which is enough to reboot the display from the
UI. Unknown keys are now logged and skipped.
Also releases the NVS handle on the validation-failure path, which returned without
calling
end().Testing
Built and run on an 8 module ESP32-C3 display. Mismatched-key case exercised by
posting a settings body containing a key the firmware does not define.
Touches
JsonSettings.cppalongside the<sstream>PR; trivial to rebasewhichever lands second.