Shill: ShillServiceClient destroys ShillClientHelpers when they are not need.

Shill: ShillServiceClient destroys ShillClientHelpers when they are not need.

This implements a removal mechanism in the ShillServiceClient where a
given ShillClientHelper is keep cached in the helpers_ std::map if at
least one of the following two condition holds:
 * There is an observer registered for the given service
 * There is a method call waiting for its answer or error response.
When a ShillHelperClient is removed also the associated ObjectProxy
is removed freeing in that way the match rules.

BUG=chromium:170182

[deymo, 2013-02-06 00:12:39]

Ok. The third attempt to fix this... and I hope, the last (although I don't
think so :) ).
I implemented Ryo's idea of having a wrapper to the callbacks in order to remove
the match rules.
I ran this patch in a real machine and I can see with dbus-monitor the match
rules going away =D.

As I said before, reference counting the helper won't work unless we call
RemoveObjectProxy in the ~ShillClientHelper... and I'm not sure if we can do
that with the current implementation.

[stevenjb, 2013-02-06 01:05:53]

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_client_he...
File chromeos/dbus/shill_client_helper.h (right):

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_client_he...
chromeos/dbus/shill_client_helper.h:70: // not.
nit: // Returns true if there is a PropertyChangedObserver registered
(see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...)

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_client_he...
chromeos/dbus/shill_client_helper.h:75: // not *other* calls waiting for a
response, this function returns false.
// Returns true if there are method calls waiting for a response. If this
function is called from within the return callback returns true only if there
are *other* calls waiting for a response.
(Don't combine 'returns true' and 'returns false', it's challenging to parse)

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_client_he...
chromeos/dbus/shill_client_helper.h:192: int ongoing_calls;
Missing trailing _
Also, maybe use pending_calls_

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
File chromeos/dbus/shill_service_client.cc (right):

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
chromeos/dbus/shill_service_client.cc:45: // a wrapper that attachs a clousure
to the given callback. It allows to call a
s/It allows to/This allows it to/

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
chromeos/dbus/shill_service_client.cc:60:
AttachClosureToDictionaryValueCallabck(
s/abck/back

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
chromeos/dbus/shill_service_client.cc:148: weak_ptr_factory_.GetWeakPtr(),
service_path)));
Rather than passing HelperStatusUpdate in each of these, could we write
AttachClosureToDictionaryValueCallback, etc, to do that binding instead?

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
chromeos/dbus/shill_service_client.cc:309: // it in such case.
nit: s/and this function removes it in such case// (implied)

[hashimoto, 2013-02-06 03:29:06]

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_client_he...
File chromeos/dbus/shill_client_helper.cc (right):

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_client_he...
chromeos/dbus/shill_client_helper.cc:28: weak_ptr_factory_(this) {
nit: Could you add ALLOW_THIS_IN_INITIALIZER_LIST here while you are at this
code?

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_client_he...
chromeos/dbus/shill_client_helper.cc:42: return ongoing_calls > 0;
How about adding another base::WeakPtrFactory dedicated for method calls and
returning its HasWeakPtrs() here?
This way, you don't need to increment and decrement |ongoing_calls| manually.

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
File chromeos/dbus/shill_service_client.cc (right):

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
chromeos/dbus/shill_service_client.cc:163: AttachClosureToClosure(callback,
I think ScopedClosureRunner runs the closure on its destruction even when the
callback is not called.
Can we go without AttachClosureToErrorCallback?

BTW, could you make a change for shill_service_client_unittest.cc to test this
behavior?

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
chromeos/dbus/shill_service_client.cc:310: void HelperStatusUpdated(const
dbus::ObjectPath& service_path) {
nit: This method should better named MaybeRemoveHelper or something?
Our style guide requires method names to be verb.

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
chromeos/dbus/shill_service_client.cc:316: if (!helper->IsObserved() and
!helper->IsWaitingResponse()) {
nit: s/and/&&/

[deymo, 2013-02-06 05:44:44]

Thank you very much for your comments!

I uploaded a new patch with the nits and other issues fixed (in some cases with
modifications). There are mainly two open questions:
1. What we do with the ScopedClosureRunner running twice our callback
2. Move the Bind to another function (I proposed a service client's method).

If we solve this soon I would like to commit this fix before the M27 branch,
since this is could eventually produce a DoS in our dbus message system.
Thanks again!

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_client_he...
File chromeos/dbus/shill_client_helper.cc (right):

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_client_he...
chromeos/dbus/shill_client_helper.cc:28: weak_ptr_factory_(this) {
On 2013/02/06 03:29:06, hashimoto wrote:
> nit: Could you add ALLOW_THIS_IN_INITIALIZER_LIST here while you are at this
> code?

Done.

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_client_he...
chromeos/dbus/shill_client_helper.cc:42: return ongoing_calls > 0;
I need this to return false in the case of the last return (to allow the
callback detect and delete the object).
I'm not a WeakPtr expert, but, I think that during the destructor of the
ScopedCallbackRunner the previous callback is not yet destroyed, so it will not
return 0... right?
It think that the counter is simple and straightforward.

On 2013/02/06 03:29:06, hashimoto wrote:
> How about adding another base::WeakPtrFactory dedicated for method calls and
> returning its HasWeakPtrs() here?
> This way, you don't need to increment and decrement |ongoing_calls| manually.

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_client_he...
File chromeos/dbus/shill_client_helper.h (right):

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_client_he...
chromeos/dbus/shill_client_helper.h:70: // not.
On 2013/02/06 01:05:53, stevenjb (chromium) wrote:
> nit: // Returns true if there is a PropertyChangedObserver registered
> (see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...)

Done.

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_client_he...
chromeos/dbus/shill_client_helper.h:75: // not *other* calls waiting for a
response, this function returns false.
On 2013/02/06 01:05:53, stevenjb (chromium) wrote:
> // Returns true if there are method calls waiting for a response. If this
> function is called from within the return callback returns true only if there
> are *other* calls waiting for a response.
> (Don't combine 'returns true' and 'returns false', it's challenging to parse)
> 

Done.

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_client_he...
chromeos/dbus/shill_client_helper.h:192: int ongoing_calls;
On 2013/02/06 01:05:53, stevenjb (chromium) wrote:
> Missing trailing _
> Also, maybe use pending_calls_
> 
More than "pending_calls" is pending "returns". The "call" part is already done.
unreturned_calls_ ?

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
File chromeos/dbus/shill_service_client.cc (right):

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
chromeos/dbus/shill_service_client.cc:45: // a wrapper that attachs a clousure
to the given callback. It allows to call a
On 2013/02/06 01:05:53, stevenjb (chromium) wrote:
> s/It allows to/This allows it to/

Done.

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
chromeos/dbus/shill_service_client.cc:60:
AttachClosureToDictionaryValueCallabck(
On 2013/02/06 01:05:53, stevenjb (chromium) wrote:
> s/abck/back
=)

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
chromeos/dbus/shill_service_client.cc:148: weak_ptr_factory_.GetWeakPtr(),
service_path)));
On 2013/02/06 01:05:53, stevenjb (chromium) wrote:
> Rather than passing HelperStatusUpdate in each of these, could we write
> AttachClosureToDictionaryValueCallback, etc, to do that binding instead?
If we do that we still need to pass the service_path and the
ShillServiceClientImpl object reference to each one of those functions.

I would prefer something more like a
ShillServiceClientImpl::HelperStatusUpdatedCallback(service_path) that returns a
base::Closure.

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
chromeos/dbus/shill_service_client.cc:163: AttachClosureToClosure(callback,
Yes, the ScopedClosureRunner will run allways... but, in this case it is safe to
call HelperStatusUpdated several times.

Anyway, I think I can simplify this a little if instead of a ScopedClosureRunner
we just run both callbacks in the RunDictionaryValueCallback (and the other). So
the body of that function will be like:
RunDictionaryValueCallback(const ...& closure, const ...& callback,
more_arguments) {
  callback.Run(more_arguments);
  closure.Run();
}
Does it look better for you?

On 2013/02/06 03:29:06, hashimoto wrote:
> I think ScopedClosureRunner runs the closure on its destruction even when the
> callback is not called.
> Can we go without AttachClosureToErrorCallback?
> 
> BTW, could you make a change for shill_service_client_unittest.cc to test this
> behavior?

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
chromeos/dbus/shill_service_client.cc:310: void HelperStatusUpdated(const
dbus::ObjectPath& service_path) {
I think the name should be more like "OnHelperStatusUpdate" because, indeed, is
a callback.

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
chromeos/dbus/shill_service_client.cc:316: if (!helper->IsObserved() and
!helper->IsWaitingResponse()) {
On 2013/02/06 03:29:06, hashimoto wrote:
> nit: s/and/&&/
Very expressive method names + too much time coding python --> "and"
=)

[stevenjb, 2013-02-06 17:10:20]

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_client_he...
File chromeos/dbus/shill_client_helper.h (right):

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_client_he...
chromeos/dbus/shill_client_helper.h:192: int ongoing_calls;
On 2013/02/06 05:44:45, deymo wrote:
> On 2013/02/06 01:05:53, stevenjb (chromium) wrote:
> > Missing trailing _
> > Also, maybe use pending_calls_
> > 
> More than "pending_calls" is pending "returns". The "call" part is already
done.
> unreturned_calls_ ?
> 
nit: Hm, 'unreturned' works, but sounds a bit awkward. 'in_progress_calls'? Or
'pending_callbacks'?

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
File chromeos/dbus/shill_service_client.cc (right):

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
chromeos/dbus/shill_service_client.cc:148: weak_ptr_factory_.GetWeakPtr(),
service_path)));
On 2013/02/06 05:44:45, deymo wrote:
> On 2013/02/06 01:05:53, stevenjb (chromium) wrote:
> > Rather than passing HelperStatusUpdate in each of these, could we write
> > AttachClosureToDictionaryValueCallback, etc, to do that binding instead?
> If we do that we still need to pass the service_path and the
> ShillServiceClientImpl object reference to each one of those functions.
> 
> I would prefer something more like a
> ShillServiceClientImpl::HelperStatusUpdatedCallback(service_path) that returns
a
> base::Closure.

That sounds like it would be fine. It's definitely optional, there's just a lot
of copy/paste code here. It might be unavoidable, but anything we can do to
simplify it (which may be a bit subjective) would help.

[deymo, 2013-02-07 00:33:15]

Hi!
The third patch is uploaded. What's new:
1. I added two checks for the add/remove observers to ensure the helper is
destroyed after removal of the observer. (The test indeed worked as exposing a
little problem: I wasn't calling the OnHelperStatusUpdate on removal =) ).
2. I removed the ScopedClosureRunner and replace it by just closure.Run() after
running the callback. This simplifies a little bit the code, and the double call
(which wasn't really a problem).
3. Added HelperStatusUpdateCallback to avoid the copy-pasted code.
4. A TODO to delay the PropertyChange until the first add is called (I would
like to not include that change in this CL).

So.. happy reviewing. =)

[stevenjb, 2013-02-07 01:12:30]

https://codereview.chromium.org/12220025/diff/10001/chromeos/dbus/shill_servi...
File chromeos/dbus/shill_service_client.cc (right):

https://codereview.chromium.org/12220025/diff/10001/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client.cc:66: 
So, I apologize for continuing to iterate on this, I know we want to get this in
for 26, but I still find this really confusing. Couldn't we do something like
this:

void ShillServiceClientImpl::SuccessDictionaryValueCallback(service_path,
callback, argument) {
  callback.Run(DBUS_METHOD_CALL_SUCCESS, argument);
  OnHelperStatusUpdate(service_path);
}

void ShillServiceClientImpl::ErrorDictionaryValueCallback(service_path,
callback, error_name, error_message) {
  OnGetPropertiesError(service_path, callback, error_name, error_message);
  OnHelperStatusUpdate(service_path);
}

// similar for ListValue and Void

Then, the code below would look something like:


  GetHelper(service_path)->CallDictionaryValueMethodWithErrorCallback(
    &method_call,
    base::Bind(&ShillServiceClientImpl::SuccessDictionaryValueCallback,
               weak_ptr_factory_.GetWeakPtr(), service_path, callback),
    base::Bind(&ShillServiceClientImpl::ErrorDictionaryValueCallback,
               weak_ptr_factory_.GetWeakPtr(), service_path, callback));

Then we wouldn't need all of these rather confusing AttachCloserTo* wrappers.

What do you think?

https://codereview.chromium.org/12220025/diff/10001/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client.cc:290: // useless.
This doesn't seem too difficult, I think it's worth doing now; I'm a little
concerned about the overhead of adding/removing the Monitoring. It also seems
like code that should be in the Helper anyway; we just need to implement
StartMonitorPropertyChanged() and make it keep track of whether or not it's
already been started.

[hashimoto, 2013-02-07 05:20:31]

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_client_he...
File chromeos/dbus/shill_client_helper.cc (right):

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_client_he...
chromeos/dbus/shill_client_helper.cc:42: return ongoing_calls > 0;
On 2013/02/06 05:44:45, deymo wrote:
> I need this to return false in the case of the last return (to allow the
> callback detect and delete the object).
> I'm not a WeakPtr expert, but, I think that during the destructor of the
> ScopedCallbackRunner the previous callback is not yet destroyed, so it will
not
> return 0... right?
You're correct.
> It think that the counter is simple and straightforward.
Counter is simple but it comes with no maintainability.
You can make a new hard-to-debug resource management bug by simply adding a new
method carelessly.
I'd like to avoid this if possible.
> 
> On 2013/02/06 03:29:06, hashimoto wrote:
> > How about adding another base::WeakPtrFactory dedicated for method calls and
> > returning its HasWeakPtrs() here?
> > This way, you don't need to increment and decrement |ongoing_calls|
manually.
>

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
File chromeos/dbus/shill_service_client.cc (right):

https://codereview.chromium.org/12220025/diff/1/chromeos/dbus/shill_service_c...
chromeos/dbus/shill_service_client.cc:310: void HelperStatusUpdated(const
dbus::ObjectPath& service_path) {
On 2013/02/06 05:44:45, deymo wrote:
> I think the name should be more like "OnHelperStatusUpdate" because, indeed,
is
> a callback.

Callback methods are named "OnXXX" when it is hard to describe its behavior with
a short phrase.
On the other hand, this method's functionality can be described with one phrase,
"(maybe) delete a helper".
Giving a meaningful names for methods is a good practice, will help people
visiting this code in future a lot.

https://codereview.chromium.org/12220025/diff/10001/chromeos/dbus/shill_servi...
File chromeos/dbus/shill_service_client.cc (right):

https://codereview.chromium.org/12220025/diff/10001/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client.cc:66: 
On 2013/02/07 01:12:30, stevenjb (chromium) wrote:
> So, I apologize for continuing to iterate on this, I know we want to get this
in
> for 26, but I still find this really confusing. Couldn't we do something like
> this:
Wasn't the bug marked as P1/ReleaseBlocker?
I think the fix would be worth merging even after the M26 branch cut.
> 
> void ShillServiceClientImpl::SuccessDictionaryValueCallback(service_path,
> callback, argument) {
>   callback.Run(DBUS_METHOD_CALL_SUCCESS, argument);
>   OnHelperStatusUpdate(service_path);
> }
> 
> void ShillServiceClientImpl::ErrorDictionaryValueCallback(service_path,
> callback, error_name, error_message) {
>   OnGetPropertiesError(service_path, callback, error_name, error_message);
>   OnHelperStatusUpdate(service_path);
> }
> 
> // similar for ListValue and Void
> 
> Then, the code below would look something like:
> 
> 
>   GetHelper(service_path)->CallDictionaryValueMethodWithErrorCallback(
>     &method_call,
>     base::Bind(&ShillServiceClientImpl::SuccessDictionaryValueCallback,
>                weak_ptr_factory_.GetWeakPtr(), service_path, callback),
>     base::Bind(&ShillServiceClientImpl::ErrorDictionaryValueCallback,
>                weak_ptr_factory_.GetWeakPtr(), service_path, callback));
> 
> Then we wouldn't need all of these rather confusing AttachCloserTo* wrappers.
> 
> What do you think?

Or, simply keep helpers alive as we did, unless the size of |helpers_| exceeds a
certain threshold (64, 128, 256, or whatever appropriate)
When it exceeds the threshold, we can delete all unneeded helpers.
(Eventually, this looks like a LRU approach which we have abandoned because it
was too complicated. But the current change looks more complicated than it.)

[satorux1, 2013-05-08 04:05:05]

closing as this is very old.