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

cjson memory leak (overload) #1722

Closed
aircable opened this issue Jan 13, 2017 · 8 comments
Closed

cjson memory leak (overload) #1722

aircable opened this issue Jan 13, 2017 · 8 comments

Comments

@aircable
Copy link

8<------------------------ BUG REPORT -----------------------------------------

Expected behavior

cjson.encode( text ) should always create the encoded string

Actual behavior

in low load situation is mostly does that, but when confronted with lots of strings to encode and then publish on an MQTT channel, it fails, and never recovers. A collectgarbage() does not help.
It is probably a memory leak. If you have ideas how to show that during test, please let me know.

Test code

Provide a Minimal, Complete, and Verifiable example which will reproduce the problem.

function gate.mesh( gateway_id, rssi, dataarray )
    meshtopic = NETID
    log( "publish "..meshtopic )
    log( "gateway "..gateway_id )
    log( "rssi "..rssi)
    log( "data "..dataarray )
    if MQTT ~= nil then
        -- decode the array
        local data = cjson.decode( dataarray )
        -- encode to JSON
        local json_txt = { gateway=gateway_id, rssi=rssi, mesh=data }
        local ok, encoded = pcall( cjson.encode, json_txt )
        log( "PUB "..encoded )
        -- topic, payload, QOS, retain flag, call done
        if ok then
            MQTT:publish( meshtopic, encoded, 0, 0, function(conn) end)
        else
            collectgarbage()
            print( "\r%6" )
        end
    end
end

This is how the function gets called, e.g.

gate.mesh(51208,-81,"[141,52,129,16,222,72,229,129,204,72,100,50,129,144,189,101,36,239,146,7,188,54,169,25,67,145,7]")

This is the debug output. Every ">" represents a function call to this function. After the "%6" is does not recover and you have to reboot.

%3
> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >> %3
> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > %6
> > %6
> > %6

NodeMCU version

Which branch are you on? If you know the Git revision then add it here as well.
Using https://nodemcu-build.com/ with cjson and mqtt modules.

Hardware

Describe which ESP8266 device you use and document any special hardware setup
required to reproduce the problem.

We build our own board with the ESP8266. Baud rate is 9600 baud. No special hardware
is required. It is just a function call, happens very often via serial command line interface.

8<------------------------ END BUG REPORT -------------------------------------

@jmattsson
Copy link
Member

The cjson library makes a lot of assumptions about memory allocations never failing, which we can't fix without pretty much rewriting the whole library. We did a best-effort patch by hooking it into the Lua memory allocations and raising an error on failed allocation, but that can still lead to leaks due to the way cjson was designed.

I believe @pjsg is currently working on a streaming json library which would side-step all of cjson.

@marcelstoer
Copy link
Member

I believe @pjsg is currently working on a streaming json library

Not sure if he's working on it but the issue is #1666 😉

@pjsg
Copy link
Member

pjsg commented Jan 17, 2017

I am (very slowly) working on it. However, my day job is taking all my bandwidth at the moment....

@aircable
Copy link
Author

aircable commented Jan 17, 2017 via email

@pjsg
Copy link
Member

pjsg commented Jan 19, 2017

For decoding, I've made a lot of progress. There is a significant open issue around string encoding (utf8 versus 8-bit characters). I'm not sure if lua5.3 affects this....

@pjsg
Copy link
Member

pjsg commented Jan 22, 2017

I have just filed a PR #1755 that creates a new JSON encoder/decoder (that can be used as a dropin replacement for cjson). I would be interested to know if it fixes your problem. I did try hard to make sure that it was memory safe and didn't leak. I'm not claiming that it is bug free, but if you find issues, then I am motivated to fix them (in the sjson code).

@marcelstoer
Copy link
Member

SJSON was merged a while ago -> closing

@aircable
Copy link
Author

aircable commented Mar 29, 2017 via email

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

No branches or pull requests

4 participants