Fixes a small memory leak when a time out is received. #430

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@seangeo

When a timeout is triggered in TimeoutCommand, disconnecting
the commandFinished slot from the underlying command results
in the response object generated by the command to never be
free'd. This change keeps the connection in place but sets a
flag on timeout that controls whether the response will be
emitted or deleted.

@mhoran

Good catch. Did this manifest itself in any way?

I'm wondering if this approach actually introduces a possible issue in leaving the signal connected. We delete the TimeoutCommand in Connection when finished is emitted. If the command has timed out, but the command itself has not yet finished, do we run the risk of triggering commandFinished after the TimeoutCommand has already been deleted? Or, because the TimeoutCommand was deleted via deleteLater, will there be no pending connections left?

@seangeo

I noticed this leak while investigating memory growth in long running capybara-webkit processes we use on Still Alive. Since it's pretty small I'm not sure that it's the cause of the problem but fixed it anyway, I'll see how it affects memory usage over the next couple days.

As I understand it, using deleteLater will not actually delete the object until all pending signals have been sent. It puts a "delete" event at the end of the queue and any pending events are removed from the queue when the delete event for the object is processed. I guess this does pose an edge case, where the TimeoutCommand gets deleted before the commandFinished signal from the child command is sent. I didn't see it in my tests but then I know we've both experienced quite different things with the tests for this stuff, OSX vs Linux differences or perhaps just a matter of timing maybe?

I can think of two alternatives to this approach, 1) we attach the child command's commandFinished signal to another object that only serves to receive the signal, delete the response and delete itself, sort of like a response sink object. Or 2) we make Response a QObject whose parent is the originating command, then when the command is deleted its response will also be deleted. (2) might be the more idiomatic QT way of doing it.

I'm happy to change this to either of those solutions if you like, assuming they work. :)

@mhoran

I like (2) best. I've been getting in the habit of setting the parent on QObjects, as the automatic memory management is quite nice. I also prefer that to manual memory management, even with deleteLater. If you'd be willing to work on turning Response into a QObject and setting up the object hierarchy, that'd be fantastic.

@seangeo

👍 Sounds good.

@seangeo

@mhoran Just updated this to make Response a QObject. Also verified that it resolves the memory leak.

Much nicer too. 👍

@mhoran

Looks great, just left a few comments. Thanks!

@seangeo

Thanks @mhoran, I'm going to revise a couple things based on your feedback, and update the PR later today.

@seangeo seangeo commented on the diff Dec 12, 2012
src/Connection.cpp
@@ -43,13 +43,15 @@ void Connection::pendingLoadFinished(bool success) {
void Connection::writePageLoadFailure() {
m_pageSuccess = true;
QString message = currentPage()->failureString();
- writeResponse(new Response(false, message));
+ Response *response = new Response(false, message, this);
+ writeResponse(response);
+ delete response;
@seangeo
seangeo Dec 12, 2012

Not sure if this should be delete or deleteLater, or if matters in this context?

@mhoran
mhoran Dec 13, 2012

I think delete is fine.

@seangeo

@mhoran updated with the changes to command parent relationships.

@mhoran

One last nitpick. emit finished(new Response(..., this)) is a bit repetitive. Perhaps Command could have an emitFinished method which did this?

@seangeo

👍

@seangeo

@mhoran Refactored to use emitFinished.

@seangeo seangeo Fix memory leak of response.
Turns Response into a QObject and sets parent to the
command that emits it.

Each Command is also a child of the decorator commands,
Timeout and PageLoading commnds, so that deleting the
top level command will delete all the children.

See discussion in #430.
82dd664
@mhoran mhoran added a commit to mhoran/capybara-webkit that referenced this pull request Dec 14, 2012
@seangeo seangeo Fix memory leak of response.
Turns Response into a QObject and sets parent to the
command that emits it.

Each Command is also a child of the decorator commands,
Timeout and PageLoading commnds, so that deleting the
top level command will delete all the children.

See discussion in #430.
77811ca
@mhoran

Merged. Thanks!

@mhoran mhoran closed this Dec 14, 2012
@mhoran

This had the fortunate side effect of masking the segfaults experienced by some users in 0.13.0. bca84f9 should fix them permanently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment