RippeR37/libbase

Implement more Callback helpers

RippeR37 opened this issue · 3 comments

RippeR37 commented

Add support for more base::*Callback helpers such as:

  • base::*Callback::Then()
  • base::BindPostTask()
  • base::SplitOnceCallback()
  • base::BarrierCallback()
  • base::BarrierClosure()
RippeR37 commented

Regarding base::BindPostTask():

Currently in Chromium (commit: 1f5633acaa6d3a27dcaaf1ea44de2685bc963e1f) BindPostTask() differs in the implementation and behavior guarantees from a similar functionality exposed as base::TaskRunner::PostTaskAndReply() by the fact that if the bound callback (reply in case of PostTaskAndReply()) dies because of inability to post-task it to the bound task runner, it will be destroyed on the task runner on which the wrapped callback is destroyed (from which the post-task was attempted). On the other hand, PostTaskAndReply() will leak such callback to avoid accidental triggering of any DCHECKs or SEQUENCE_CHECKERs in the destructors.

It might have been originally though that this may be visible only during shutdown (so possibly better to leak then crash), but in fact this can also happened whenever a thread is stopped from which PostTaskAndReply() was done and before the reply was queued.

As such, the behavior in the original //base implementation is not aligned between helpers which makes it harder to use them (or could lead to bugs due to implicit assumptions between them).

Due to this reason, extra analysis will be done before implementing this helper to decide which path to choose:

  1. Keep the same behavior as original //base with warnings for users
  2. Make both helpers leak bound helpers that couldn't have been post-tasked (change BindPostTask() behavior)
  3. Make both helpers destroy callbacks on possibly wrong thread/sequence, always (not leaking anything, but changing PostTaskAndReply() behavior)
  4. Implement both versions with compile-time switches/defines and let the end-user decide (some default behavior would still have to be chosen though).

Initially I'm leaning toward (3) as that would be the "clean" solution that doesn't inherently leak anything in its design, albeit it could lead to hitting DCHECKs if one is not careful enough.
As said - this requires more analysis of scenarios which could lead to such problems and if there are ways to avoid them.

RippeR37 commented

After some consideration, I've decided that libbase will - by default - clean up all resources. This may lead to hitting some DCHECKs/SEQUENCE_CHECKER's checks, but it should be possible to work around them. For compatibility, there might be added a compile-time switch (most likely in a form of #define) to revert to Chromium //base module's behavior: leaking (in that specific scenario).

Highest care should be taken on proper resource management and corectness. Leaking by design doesn't seem to be a sane approach and in some potential corner cases, it would still be possible to do this (leak) in that specific case, without affecting default behavior. Leaking might be viable only during the process shutdown procedure, but the problem appears also when a thread (or thread pool) is stopped which might happen at any time during application's lifetime.

RippeR37 commented

If the users if libbase will define LIBBASE_POLICY_LEAK_ON_REPLY_POST_TASK_FAILURE, the implementation will fallback to the "original" behavior (leaking if the post-task-back-to-original-sequence fails). Default and currently only supported option is to destroy the callback on the "other" task runner and implementations are expected to prefer this behavior to ensure no memory is leaking.