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

Issue #21 - Memory leak #22

Closed
wants to merge 3 commits into from
Closed

Issue #21 - Memory leak #22

wants to merge 3 commits into from

Conversation

watsocd
Copy link

@watsocd watsocd commented Aug 7, 2015

This is my first contribution to an open source project and my first use of GitHub so if I did anything incorrectly, just delete this entire thing.

If the remote server does not respond to the HTTP request, line 223 (for example) returns from the PUT function before doing the CURL cleanups on lines 229 thru 231. This causes a memory leak.

I tested the following code change with the PUT function and it resolves the leak. I made changes in all functions that had similar code.

if (res != CURLE_OK)
{
ret.body = "Failed to query.";
ret.code = -1;
//return ret; //Commented the return
} else { //else added here
long http_code = 0;
curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &http_code);
ret.code = static_cast(http_code);
}
...Followed by CURL cleanup and return statement.

Functions now have only one return statement.

Chuck and others added 3 commits August 7, 2015 10:59
…s from the PUT function before doing the CURL cleanups on lines 229 thru 231. This causes a memory leak.

I tested the following code change with the PUT function and it resolves the leak. I made changes in all functions that had similar code.

if (res != CURLE_OK)
{
ret.body = "Failed to query.";
ret.code = -1;
//return ret; //Commented the return
} else { //else added here
long http_code = 0;
curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &http_code);
ret.code = static_cast(http_code);
}

Functions now have only one return statement.
@watsocd
Copy link
Author

watsocd commented Aug 7, 2015

I looked at the failing checks and I don't believe they are related to the changes I made.

@mrtazz
Copy link
Owner

mrtazz commented Aug 9, 2015

@watsocd thank you so much for taking the time to contribute! The tests have had some problems which I was meaning to look into, so they are likely unrelated. I'll take a closer look at the changes in the coming days and will comment then.

@mrtazz
Copy link
Owner

mrtazz commented Aug 9, 2015

@watsocd I changed master to fix the tests. Would you mind to merge it so we can have the build succeed here?

@mrtazz
Copy link
Owner

mrtazz commented Aug 9, 2015

also this fixes #21, right?

@watsocd
Copy link
Author

watsocd commented Aug 11, 2015

The one leak issue was already fixed. This second issue takes care of a second leak in the same code.

I looked at the testing code. Are we positive the test server is functional?

@watsocd
Copy link
Author

watsocd commented Aug 17, 2015

I am new to Github. It won't let me merge. It says:

This branch is up-to-date with the base branch
Only those with write access to this repository can merge pull requests.

@mrtazz
Copy link
Owner

mrtazz commented Aug 18, 2015

ah yea I meant merge master into your pull request branch. It should go something like this:

git pull https://github.com/mrtazz/restclient-cpp.git master
git push https://github.com/watsocd/restclient-cpp.git master

Does that help?

@watsocd watsocd closed this Aug 20, 2015
@watsocd watsocd reopened this Aug 20, 2015
@watsocd
Copy link
Author

watsocd commented Aug 20, 2015

Looks like it passed now but I am not sure I did anything!! I am finding Git extremely confusing!!

@mrtazz
Copy link
Owner

mrtazz commented Aug 21, 2015

hmm that's interesting. Not sure why they pass now. It looks like your branch is still using the old test setup which should be completely defunct. But don't worry about that. Can you just merge in master and write a test to run all methods against a non-existing URL maybe so we can have tests running against this new functionality?

@mrtazz mrtazz added this to the v0.3.0 milestone Sep 3, 2015
@mrtazz
Copy link
Owner

mrtazz commented Dec 26, 2015

@watsocd any updates on this?

@mrtazz mrtazz removed this from the v0.3.0 milestone Jan 10, 2016
@mrtazz
Copy link
Owner

mrtazz commented Feb 2, 2016

this should be fixed via #39 now

@mrtazz mrtazz closed this Feb 2, 2016
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.

2 participants