Hi Amitay,

I just noticed
https://git.samba.org/?p=amitay/samba.git;a=commitdiff;h=e63c7ab81a74598d08b454ab

I think that's not the correct fix.

There quite some layer violations (also in the current code).

sock_daemon_context_destructor() is the caller of
sock_daemon_run_send/recv and it should never call
tevent_req_done(). It should just call TALLOC_FREE(sockd->req);
Or at most it can call tevent_req_received(sockd->req).
BTW: eventd_cancel_running() seems to have a similar problem.

In the implementation of sock_daemon_run_* you're not following
the typical naming pattern for tevent_req based functions.

The state in sock_daemon_run_send should be named
sock_daemon_run_state and all step functions should
name a sock_daemon_run_ prefix. That makes it much clearer
what belongs to the flow between sock_daemon_run_send and
sock_daemon_run_recv.

In all cases you should never to anything after a tevent_req_done()
tevent_req_nomem() or tevent_req_*error*(), you have to call
'return;' (or return tevent_req_post(req, ev);) immediately.
There're quite some places where this is violated
and I guess that's the core reason for the problem.

If you really have to do more than return you need to use
tevent_req_defer_callback() to make it safe.

Using a cleanup function via tevent_req_set_cleanup_fn()
is the preferred way to trigger some cleanup code via
tevent_req_done, tevent_req_nomem, ... and tevent_req_received()

(In reply to Stefan Metzmacher from comment #1)

Thanks for your valuable feedback.

This code has gone through many revisions, so the function names got changed around a lot and I forgot to rename the sock_daemon_start_state to sock_daemon_run_state.  I will take care of that.

I was not aware that you cannot call anything else after tevent_req_done() or tevent_req_error().  I will update the code to avoid such cases.

I really need a way to tell the sock_daemon_run computation to finish from within any of the callbacks (either sock_daemon_funcs or sock_socket_funcs).

I can think of following two options:

1. talloc_free(sockd).  This is what I have currently implemented.
   I do  understand the layer violation issue here.

2. Introduce sock_daemon_terminate(sockd)
   Would that be the right approach?

void sock_daemon_terminate(sockd)
{
    req = sockd->req;
    state = tevent_req_data(req, ...);

    sock_daemon_shutdown(state);
    sockd->req = NULL;

    tevent_req_done(req);
}

Looking at the description of tevent_req_defer_callback(), may be I need to add tevent_req_defer_callback() before calling tevent_req_done().

void sock_daemon_terminate(sockd)
{
    req = sockd->req;
    state = tevent_req_data(req, ...);

    sock_daemon_shutdown(state);
    sockd->req = NULL;

    tevent_req_defer_callback(req, state->ev);
    tevent_req_done(req);
}

(In reply to Amitay Isaacs from comment #2)

> I really need a way to tell the sock_daemon_run computation to finish from 
> within any of the callbacks (either sock_daemon_funcs or sock_socket_funcs).

Can't they return false or some error to indicate the termination?

If somehow possible you should try to avoid the need of the ->req pointers
completely.

Created attachment 12833 [details]
Patches for v4-6

Hi Karolin,

This is ready for 4.6.

Thanks...

(In reply to Martin Schwenke from comment #5)
Pushed to autobuild-v4-6-test.

(In reply to Karolin Seeger from comment #6)
Pushed to v4-6-test.
Closing out bug report.

Thanks!