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

Expand JSON buffer lock scope to entire web reply #3648

Merged
merged 3 commits into from
Jan 6, 2024

Conversation

willmmiles
Copy link
Contributor

When generating a JSON reply, ensure that the global JSON buffer isn't released until the response is completely ack'd by the client. This fixes some cases where the JSON reply exceeded a single packet and the second packet was corrupted by later users of the global buffer overwriting some of the packet data.

Fixes #3641

Add JSONBufferGuard, an RAII lock guard class for JSONBufferLock
analogous to std::lock_guard.  This permits binding the guard to a scope,
such as an object life cycle or function body, guaranteeing that the
lock will be released when the scope exits.
Extend the JSON response class to hold the global JSON buffer lock
until the transaction is completed.

Fixes Aircoookie#3641
Ensure we delete the response if it's not locked
@blazoncek blazoncek merged commit 801d92b into Aircoookie:0_15 Jan 6, 2024
12 checks passed
blazoncek added a commit that referenced this pull request Jan 6, 2024
Expand JSON buffer lock scope to entire web reply
softhack007 pushed a commit to MoonModules/WLED that referenced this pull request Jan 8, 2024
Expand JSON buffer lock scope to entire web reply
Djelibeybi pushed a commit to Djelibeybi/WLED-MM that referenced this pull request Jan 15, 2024
Expand JSON buffer lock scope to entire web reply
@WAPEETY
Copy link

WAPEETY commented Jan 22, 2024

is it possible to have this kind of problem even after update?

@blazoncek
Copy link
Collaborator

Buffer reuse has been solved by this PR.

DedeHai pushed a commit to DedeHai/WLED that referenced this pull request Jan 27, 2024
Expand JSON buffer lock scope to entire web reply
@WAPEETY
Copy link

WAPEETY commented Jan 27, 2024

I had a broken JSON for this so I solved in this way

home-assistant/core#103723 (comment)

@blazoncek
Copy link
Collaborator

@WAPEETY presets have no connection with this issue. Presets are always served "as is" from the file system.

@WAPEETY
Copy link

WAPEETY commented Jan 27, 2024

wait so it has anything to do about the fact that I was recieving a broken JSON from the presets?

LordMike added a commit to LordMike/WLED that referenced this pull request Feb 4, 2024
@willmmiles willmmiles deleted the json-response-locking branch March 7, 2024 00:06
@jw2013
Copy link

jw2013 commented May 13, 2024

It took me quite a while to get here, and I wonder if nobody else discovered this yet:

This patch completely destroys the performance, at least on an ESP8266. Easy to check:
600 LEDs (WS2815), Power limiter on, e.g. effect Glitter.

Up to 0.14.1-b2 (d1-mini, 160 MHz), getting constant 23 FPS (even 25 FPS with 0.13.*).
With 0.14.1-b3 (d1-mini, 160 MHz) and later, getting 13 FPS and less.

Copying json.cpp from 0.14.1-b2 to 0.14.1-b3 restores the 23 FPS.

Can anybody check this please, and confirm?

@blazoncek
Copy link
Collaborator

Any function in json.cpp is only ever called when accessing presets or UI interaction or other JSON parsing. If you use those frequently FPS may drop on ESP8266. Use ESP32 in such case.

Otherwise it has no influence on FPS.

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.

4 participants