[PATCH] tevent and threads - infrastructure improvements.

[Stefan (metze) Metzmacher]

Am 23.07.2015 um 01:40 schrieb Jeremy Allison:
> On Wed, Jul 22, 2015 at 04:21:55PM -0700, Jeremy Allison wrote:
>> On Wed, Jul 22, 2015 at 04:12:08PM -0700, Jeremy Allison wrote:
>>> On Thu, Jul 23, 2015 at 12:52:15AM +0200, Stefan (metze) Metzmacher wrote:
>>>
>>>> Something like this would be better:
>>>>
>>>> struct tevent_context *tevent_thread_proxy_context(struct tevent_context
>>>> *main_ev);
>>>>
>>>> The thread can then only schedule immediate events on the returned proxy
>>>> context, this will wakeup the main_ev and move the immediate events from
>>>> the proxy to the main_ev.
>>>>
>>>> The immediate events can be preallocated in before starting the thread.
>>>
>>> I don't get this. Can you explain how this would be
>>> used in the same way as ev_async_send() ?
>>
>> More thing I don't get. Who calls tevent_thread_proxy_context() ?
>> Is it the requesting thread ? If you're returning a struct tevent_context *
>> that means internal changes to tevent_immediate when you schedule
>> to decide if it's a regular context or a "proxy" context. As I've
>> said, preallocation is a non-starter for scalable use so I think
>> I need to see sample client code before I can understand your
>> thinking here.
>>
>> Also the code I posted has no internal changes to existing
>> tevent code, which is a big plus for me.
> 
> Oh yeah, one more thing (sorry, getting a gut reaction
> to this as I think about it more :-).
> 
> Returning a "proxy" struct tevent_context *
> from a call like tevent_thread_proxy_context()
> is a really bad API decision.
> 
> It pretends to be a regular struct tevent_context *,
> but it's not in that the only thing you can do with
> it is call tevent_create_immediate() on it - you
> shouldn't be able to call tevent_add_fd() or
> tevent_add_time() on it as that wouldn't have any
> meaning. But the coder doesn't realize that - it
> just apprears to be a regular struct tevent_context *
> to them - only convention says otherwise.
> 
> That's a receipe for coding disasters IMHO.

True, maybe something like

struct tevent_thread_proxy *tevent_thread_proxy_create(struct
tevent_context *main_ev);

void tevent_thread_proxy_schedule(struct tevent_thread_proxy *proxy,
				  struct tevent_immediate **im,
				  tevent_immediate_handler_t handler,
				  void *private_data);

tevent_thread_proxy_create() replaces your
int tevent_threaded_context_register(struct tevent_context *ev);

TALLOC_FREE(proxy) replaces your
int tevent_threaded_context_unregister(struct tevent_context *ev);

tevent_thread_proxy_schedule() replaces your
tevent_threaded_async_call().

The thread creates a new struct tevent_immediate (or uses an existing one)
and allocates all related private memory under it.

tevent_thread_proxy_schedule() talloc_moves im (including the private data)
to the proxy. It notifies the main_ev and the trigger schedules the im
on the main_ev. The handler can free im or reuse it later.

Using an explicit struct tevent_thread_proxy avoids using global variables
and allows more than one proxy. This is important if some highlevel
_send/recv function
wants to use threads in the background. This will allow
gensec_update_send/recv make use
of this and call the gss_init_sec_context() in a thread.

As a helper we can also add a tevent_req_set_thread_proxy() a thread
could then use:

   tevent_req_set_thread_proxy(req, state->thread_proxy);
   tevent_req_done(req);
   return;

I'd love to use something like this in Samba :-)

metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20150723/139d5e58/signature.sig>