Refactor client reset

[blagoev]

refactor client reset

[nielsenko]

Have a few suggestions in the c++ part, but otherwise fine with me

[nirinchev]

I dislike that this is now so specific. There's nothing in the function implementation that indicates that this is somehow related to locking. It's just invoking a function that takes a boolean. The fact that in the client reset flow it'll unlock a lock is an implementation detail.

[blagoev]

This exactly a specific function to unlock and the argument is called unlockFunc, hence the otherway around was weird. This makes it more clear what it does and when it should be used.

[desistefanova]

I think we can renamed only the argument name. It has left unchanged after I refactored the function to be general.

[blagoev]

No, I very strongly disagree. We don't need and we are not trying to write a generic function that can invoke arbitrary functions that take a boolean in native code. We don't have such use case at hand. We are trying to provide a specific mechanism that when needed, can call Dart and wait for a result synchronously and needs an unlock function to continue. This unlock can be used for any generic case where we need a result from dart through the Scheduler.

[nirinchev]

I also think we should rename the argument - it's a void* which is then cast to function(bool). Nothing about it deals with unlocking anything.

[blagoev]

@nikola This function is named after what it does and not how it is used. It invokes a unlock function callback which was passed to a Dart function which accepts a unlock callback argument. This ties the whole concept together. So later when someone needs to do the same it is obvious what needs to be done. create a static dart function which should return a result using that function and continue the operation

[nirinchev]

Again, the function being passed as an argument is not an unlock function. It's a function(bool).

The fact it's an unlock function is controlled from the outside. So you're naming this function after how it's being consumed.

[nirinchev]

Consider this extreme example:

void bark(Object dog) {
  final animal = dog as Animal;
  animal.produceSound();
}

final dog = Dog();
dog.bark();

Do you think the bark function is named correctly here? Because this is what is essentially happening with this C function. We accept a generic void* pointer, we cast it to a function(bool), then invoke that function. And we call the argument unlockFunc because we expect that consumers are going to invoke us with a function that unlocks something.

[blagoev]

I don't think we are getting anywhere here. I would like to move away from bike shading the function name. The function is called invoke_unlock_callback and it does what it says and expect a specific argument to be passed in and be used in a specific case. Lets move the discussion to another place where we can gain more value.
The other feedback for deadlocks is more important in this PR. I am trying to handle it.

[nirinchev]

I'm fine with moving the discussion elsewhere, but I disagree with changing the function name here. Since both Desi and I believe the old name is better and the name change is not fixing an actual bug, let's keep the original name and we can rename it once we've completed bikeshedding.

[nirinchev]

Looks good once we revert the function name change :)