tevent_loop_set_nesting_hook deprecated?

[simo]

On Thu, 2009-03-19 at 09:48 +0100, Stefan (metze) Metzmacher wrote:
> Hi Tridge,
> 
> > I notice you marked the new interface tevent_loop_set_nesting_hook()
> > as deprecated. It is a bit of a strange idea marking a new
> > interface as deprecated :-)
> 
> :-)
> 
> > I presume you marked it as deprecated as you would like to discourage
> > new semi-async code. That doesn't really make sense though, as the
> > caller of tevent_loop_set_nesting_hook() isn't semi-async, it is just
> > adding code in case some other routine is semi-async. It is needed
> > because vfs_unixuid changes the global state of the process (the
> > euid/egid/groups) and that doesn't fit with other code that may depend
> > on a particicular value of state state (ie. depend on us running as
> > root).
> 
> I'm fine with removing the compiler warning,
> maybe I can add we can let samba4 define TEVENT_DEPRECATED_NOWARN
> As tevent will also be used by other external projects. I marked it
> as deprecated, because samba4 should be the only user of this function.
> 
> > I also don't think it is realistic to want Samba to never have any
> > semi-async code. The particular case that triggered the addition of
> > tevent_loop_set_nesting_hook() demonstrates how hard it is to make
> > everything completely async. To make it async, all internal calls in
> > all of the VFS layer would need to be fully async to cope with the
> > possible calls to winbind. That is a pretty big overhead for something
> > that is really a non-issue.
> > 
> > semi-async code is not evil. It is a heck of a lot better than
> > non-async code, and the only real downside to semi-async code is that
> > the order of completion of multiple outstanding semi-async calls is
> > the reverse order the calls were made with. 
> 
> I think semi-async code make thinks really hard to debug,
> and we added a lot of complex hacks, e.g. in the packet code
> to detect that the same event handler is called twice.
> 
> Also we have a lot of places in our client libraries,
> where we just crash when we get timeouts, because the
> timeout event is triggered and calls the higherlevel completion
> function, which then destroys a request which is still
> actively processed in the nested event stack.

Same feeling here.

Processing more than 1 event at a time is a recipe for madness
timeout handling is not the only thing that go bonkers, think of an
event that triggers a change in configuration, If that happens within a
nested event you then have to go to great length to assure the calling
event is not dependent on the configuration you are changing.
Results can range from simple misbehavior to segfault because you are
referencing stuff that has been freed and replaced etc...

> > The existance of a semi-async call does nothing at all to harm
> > full-async code. The full-async code can continue to operate
> > completely correctly in the presence of any number of outstanding
> > semi-async calls, so the only routines that can be adversely affected
> > by semi-async calls are themselves.
> 
> That's not true, see my example above, it removes that garantee that
> only one event is processed at a time. As that makes sure that a timeout
> handler is triggered not as the same time the main working function of
> the request is handled. And adding complexity handle that in every
> full-async code, is really ugly!

And doomed to fail.
Other risks are also excessive nesting, I guess on modern linux systems
the stack is deep enough to allow for a lot of nested events, not sure
about other platforms though.

> > This is much better than the non-async approach that we will be left with if semi-async is
> > deprecated. If a non-async call is made, then everything else
> > stops. That should indeed be avoided, and if you want to deprecate
> > that then I'd fully support you. Deprecating the much better solution
> > of using semi-async makes no sense to me.
> 
> Samba4 should just ignore the deprecation warning (or we just remove
> it), but it should need the TEVENT_DEPRECATED define to get that function.
> 
> I know we need to support semi-async code for while, but with the
> tevent_req infrastructure it's relatively easy to convert the semi-async
> code to become full-async, which is the best solution.
> 
> I know it's a lot of work to convert all semi-async code, but I think
> that should be the end goal.

I fully agree, if you decide to go async, than so be it, make the code
fully async.

Simo.


-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>