On 04/16/2010 06:17 PM, Simo Sorce wrote:
On Fri, 16 Apr 2010 10:02:08 +0400
Eugene Indenbom<eindenbom(a)gmail.com> wrote:
> I agree that it would be good to prevent or at least limit recursion
> of callbacks, but unfortunately as callbacks are called from
> destructor it is not possible to resolve within this patch.
>
Well, I'll think about that.
> IMHO it is very bad policy to call callbacks from destructor. My
> patch #4 resolves the issue by introducing reference counting of
> sdap_handle.
>
calling callbacks from a destructor is perfectly fine, it is the method
used to clean-up and reply to callers if something went wrong, I don't
see anything bad with that.
Well, it is possible not to call callbacks from destructor. The
technique is implemented in my patch #4:
1. Each active operation holds a reference on sdap_handle (or on
sdap_id_conn_data to be precise)
2. When connection breaks sdap_handle_release(final = false) starts
calling callbacks
3. Within each callback a reference on sdap_handle is dropped
4. Within last callback the last reference on sdap_handle is dropped
5. sdap_handle is destroyed
6. sdap_handle_release(final = true) is called, but there is no ops to
notify
This way it is possible to avoid calling callbacks from destructor.
What is important is that callbacks do not attempt to recurse
(starting
new requests) when there are errors. This is a general implicit rule
of the tevent model anyway, not something we are making up here for
this case.
> It is not possible to call callback in outer
> sdap_handle_release(final = false), as all callbacks must complete
> before sdap_handle is destroyed (that is sdap_handle_release(final =
> true) completes).
>
> On the other hand it is an absolute must to prevent double
> destruction of sdap_handle. This could be achieved by fixing
> talloc_zfree. Therefore in worst case the recursion is limited by to
> levels:
> - sdap_handle_release(final = false)
> - sdap_handle_release(final = true)
>
> The third level of sdap_handle_release(final = true) would mean
> double destruction, that is obviously a disaster.
>
Ok, let me try to summarized what is the problem and what we need to do
in abstract here, because we are going down a rat hole of exceptions
and I think we are loosing the point.
There are only 2 ways for sdap_handle_release() to be called.
1. from sdap_process_result() in case the ldap handler returns a fatal
error.
2. from sdap_handle_destructor when someone tries to free the handle.
In any case what we want is to signal all pending requests that they
need to terminate either because we want to close the connection
immediately or because there was an error.
In any case, ideally we never want sdap_handle_release to recurse,
because we are already cleaning up and recursing causes issues.
The only difference between the 2 patch to get to sdap_handle_release
is that if the destructor calls it we want the handle to be freed at
the end of the outermost loop. While if we call it only from
sdap_process_result we do not want to free it because other functions
may still reference ctx->gsh, so we just want to release everythin but
the memory context itself and only to avoid segfault if callers try to
dereference it. But no caller will ever use the handle anymore because
sh->connected is false.
Is there anything I am missing here ?
Looks correct. Being a bit paranoid, I would like to state is in my own
words.
There is 3 possible scenarios:
1. Only talloc_zfree(gsh) is called:
- sdap_handle_release(final = true) is called
- operation callbacks are called
- LDAP handle is deallocated
- sdap_handle_data is deallocated
- sdap_handle is deallocated
2. Only sdap_process_result is called:
- sdap_handle_release(final = true) is called
- operation callbacks are called
- LDAP handle is deallocated
- sdap_handle and sdap_handle_data live waiting for talloc_zfree to
free them
This scenario takes place when there is no active op, but cached
connection breaks.
3. Recursion:
- sdap_handle_release(final = false) is called
- first operation callback is called
- ...
- talloc_zfree(gsh) is called
- sdap_handle_release(final = true) is called
- further operation callbacks are called
- LDAP handle is deallocated
- sdap_handle is deallocated
- ...
- the control returns to sdap_handle_release(final = false)
- sdap_handle_data is freed
So the difference between paths:
1. If no talloc_zfree is called, sdap_handle and sdap_handle_data
survive, but otherwise connection is closed.
2. talloc_zfree destroys everything except sdap_handle_data if there is
an outer sdap_handle_release(final = false) call. Then outer call to
sdap_handle_release releases sdap_handle_data.
Looks equivalent to what you have written, a bit more verbose may be.
Eugene