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

Memory leak while sending http post request #1383

Closed
blelump opened this issue Jul 8, 2016 · 5 comments
Closed

Memory leak while sending http post request #1383

blelump opened this issue Jul 8, 2016 · 5 comments

Comments

@blelump
Copy link

blelump commented Jul 8, 2016

Expected behavior

Node heap size does not decrease while sending requests to a not working server.

Actual behavior

Node heap size continuously decreases with the amount of sent requests, eg:

HTTP request failed
heap:   43320
HTTP request failed
heap:   43136
HTTP request failed
heap:   42984
HTTP request failed
heap:   42832
HTTP request failed
heap:   42680
HTTP request failed
heap:   42520
HTTP request failed
heap:   42336
HTTP request failed
heap:   42168
HTTP request failed
heap:   42008
HTTP request failed
heap:   41840
HTTP request failed
heap:   41680
HTTP request failed
heap:   41512
HTTP request failed
heap:   41344
HTTP request failed
heap:   41184
HTTP request failed
heap:   41032
HTTP request failed
heap:   40880
HTTP request failed
heap:   40728
HTTP request failed
heap:   40560
HTTP request failed
heap:   40408
HTTP request failed
heap:   40256
HTTP request failed
heap:   40104
HTTP request failed
heap:   39952
HTTP request failed

Test code

tmr.alarm(0, 5000, tmr.ALARM_AUTO, function()
    local data = "{}"
    http.post('http://some_not_working_url',
          'Content-Type: application/json\nAccept: application/json\n',
          data,
          function(code, data)
              if (code < 0) then
                print("HTTP request failed")
              else
                print(code, data)
              end
    end)
    print('heap: ', node.heap())
end)

NodeMCU version

NodeMCU custom build by frightanic.com
    branch: master
    commit: cdaf6344457ae427d8c06ac28a645047f9e0f588
    SSL: true
    modules: adc,dht,file,gpio,http,net,node,tmr,uart,wifi
 build  built on: 2016-06-11 22:55
 powered by Lua 5.1.4 on SDK 1.5.1(e67da894)

Hardware

Info from esptool:

Manufacturer: e0
Device: 4016
@Jonathan411
Copy link

Jonathan411 commented Jul 16, 2016

I encountered this same issue and posted a test case on stackoverflow (before Marcel let me know that the issue was covered here), and also found a possible clue relating to the message body:

http://stackoverflow.com/questions/38397710/nodemcu-memory-leak-in-http-module-post

@devsaurus
Copy link
Member

A clue to the issue is that the memory decrease after each http.post call is directly related to the size of the message body of the http request. The larger the message body the greater the memory available decrease.

My best bet based on a code review:

  • Several members are allocated on heap when setting up a request.
  • These members can be assigned to 2 groups: infrastructure (hostname, method, path) and payload (headers, post_data).
  • Memory for payload members is free'd on the go after they are sent.
  • Memory for infrastructure members is free'd in http_disconnect_callback().

Conclusion:
Since no connection is established in the given scenario, the payload members are never sent and thus never free'd. Calls to os_free() should be added to conditionally free payload members in http_disconnect_callback().

I'm going to craft a PR in the next days, although my possibilities to actually test the leak's elimination are very limited atm. Anybody with a test setup is welcome to take over 😉

@marcelstoer
Copy link
Member

my possibilities to actually test the leak's elimination are very limited atm

I can do that. Any issue that doesn't require extra hardware is a good one for me 😉 The test case @blelump provided is even easier to run than the procedure @Jonathan411 described on SO.

@Jonathan411
Copy link

Jonathan411 commented Jul 17, 2016

If additional testing is required, I can run it through the test procedure I already have set up.

@marcelstoer marcelstoer removed this from the 1.5.4 milestone Jul 17, 2016
@devsaurus
Copy link
Member

@Jonathan411 yes, would be helpful to have results from another test case for confirmation.

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