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

NSAPI/lwIP: Free held netbuf on close #3975

Merged
merged 1 commit into from Mar 23, 2017

Conversation

Projects
None yet
6 participants
@kjbracey-arm
Contributor

kjbracey-arm commented Mar 21, 2017

mbed_lwip_socket_recv() takes one netbuf at a time from the netconn API,
and it holds a partially-read netbuf if necessary in order to present as
a stream for TCP.

This held netbuf was not being freed when the socket was closed.

Fixes issue #3974.

NSAPI/lwIP: Free held netbuf on close
mbed_lwip_socket_recv() takes one netbuf at a time from the netconn API,
and it holds a partially-read netbuf if necessary in order to present as
a stream for TCP.

This held netbuf was not being freed when the socket was closed.
@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Mar 21, 2017

One note on this netbuf thing for future consideration - the netbuf API isn't ideal for TCP, as TCP doesn't internally hold netbufs, so netconn_recv() itself allocates a temporary netbuf just to make TCP match UDP and raw.

As we know this path is TCP-only, you could use netconn_recv_tcp_pbuf to skip that step, and just work on the pbufs.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 21, 2017

/morph test-nightly

@0xc0170 0xc0170 added the needs: CI label Mar 21, 2017

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 21, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1709

All builds and test passed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 22, 2017

Restarted jenkins CI, please look at the result once done

The current failures are 3 identified, one of them for instance ./mbed-client/source/m2mbase.cpp:60:23: error: 'M2MBase::lwm2m_parameters_s' has no member named 'dynamic_resource_params' , is this valid? As this patch does not touch that code

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Mar 22, 2017

The branch was deliberately created based on an old revision, partly to test whether your CI was correctly testing the merge result with master, or just testing the tip of the branch.

It seems it might just be testing the tip. I could rebase the branch, but maybe you want to check your CI configuration?

@tommikas

This comment has been minimized.

Contributor

tommikas commented Mar 22, 2017

@kjbracey-arm The mbed-client part of the jenkins/pr-head build doesn't currently do the merge. There's a task to add it, but I'm not sure what the status of it is at the moment.

@tommikas

This comment has been minimized.

Contributor

tommikas commented Mar 22, 2017

@kjbracey-arm We'll fix it soon (TM). If you're in a hurry with this PR I'd suggest rebasing.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Mar 22, 2017

I'm not personally in a hurry - it's just to address the GitHub issue referenced - a slightly hard-to-cause memory leak.

So I'm happy to leave it for the moment as a testcase.

@geky

geky approved these changes Mar 22, 2017

Looks good to me 👍

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Mar 23, 2017

@0xc0170 I believe Jenkins has been fiddled with successfully. Could you retrigger?

@tommikas

This comment has been minimized.

Contributor

tommikas commented Mar 23, 2017

@kjbracey-arm @0xc0170 I just restarted the job.

@adbridge adbridge merged commit ebfe04a into ARMmbed:master Mar 23, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test-nightly Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment