[RFC] [WIP] tevent/glib integration

[Ralph Boehme]

Hi Noel,

On Mon, Dec 28, 2015 at 11:01:22AM +0000, Noel Power wrote:
> On 22/12/15 19:55, Ralph Boehme wrote:
> > Hi Noel,
> >
> > On Mon, Dec 21, 2015 at 03:07:36PM +0000, Noel Power wrote:
> >> On 16/12/15 14:08, Stefan Metzmacher wrote:
> >>> very interesting work! It would be really good to have this
> >>> problem solved cleanly in future.
> >> [...]
> >>> Instead we can use a tracecallback and hook into the
> >>> TEVENT_TRACE_BEFORE_WAIT (and maybe TEVENT_TRACE_AFTER_WAIT)
> >>> events.
> > I believe you meant the TEVENT_TRACE_BEFORE_LOOP_ONCE and
> > TEVENT_TRACE_BEFORE_AFTER_ONCE trace points.
>
> I also looked at using those but they stretch the scope of the
> prepare_poll/check_poll much much longer than I wanted. e.g. in this
> specific implementation case the glib context being acquired and held
> for the entire event_loop_once call. I was purposely trying to keep that
> to a minimum (and additionally now with your patch  the glib context
> could be held even while normal tevent events are processed, I don't
> know if that specifically can cause a problem but it looked like it
> might have potential for unwanted/unexpected sideaffects).

I don't see a problem and it greatly simplifies code. It will block
other glib threads from acquiring and using the context and that's
probably a good idea anyway. Not sure, but maybe we should acquire the
context once at the beginning and never release it.

> Also now there is more possibility for useless prepare_poll/check_poll
> cycles where the poll function might not even be called (note: in 
> tevent_poll.c the 'poll_event_loop_once' function processes timers,
> immediate events and signals first and can return before the poll
> function is even called )

I don't see a problem here.

> In the 's3' backend effectively the same thing
> can happen because 'run_events_poll' will also process timeouts/signals
> etc. first and may not process fds at all (until a subsequent iteration)
> If you want flexibility around when/where you call
> prepare_poll/check_poll the trace points imo just don't cut it.

But it keeps the tevent loop_once function simpler. I'm not arguing
for using the trace points no matter what, imo we should implement the
most simple and elegant design.

> If it was possible to ignore the points above (and maybe it is, perhaps
> I am being too careful or extreme) using the trace points imo only makes
> sense if it is possible to use them in external code, by external code I
> mean code that links to the tevent library and uses the public headers
> available from tevent. In your code the 'prepare_poll' and 'check_poll'
> code is internal to the 's3' tevent backend and thus for example the
> timeout information can be easily transferred. But.. how could the code
> called by TEVENT_TRACE_BEFORE and TEVENT_TRACE_BEFORE transfer timeout
> information to generic 'tevent' backend event_loop_once function(s),

tevent_get_foreign_poll_ctx(). That's the only modification necessary
in s3_event_loop_once(). Call tevent_get_foreign_poll_ctx() to get at
the timeout of the foreign loop.

<https://git.samba.org/?p=slow/samba.git;a=blobdiff;f=source3/lib/events.c;h=716f809712f00b7fea6e06497b61efbd1eda2923;hp=0bc56e454add8445782f95c1b94746218f19791c;hb=refs/heads/tevent-glib-spotlight;hpb=dffe228283ba756d4370753a1e2aed3cee3acccd;ds=inline>

> >> + imho it is not at all an intuitive api to use, that the fds of the
> >> poll function could be modified by these trace points isn't at all
> >> obvious even by name
> > afaict we're not modifyint the pollfds, are we?
>
> well not directly but indirectly we are, calling tevent_add_fd
> essentially is the same as modifying the pollfds isn't it ?

No. Using tevent_add_fd is a well defined interface. Directly hacking
the pollfd array is a completely different game.

> Also now because you removed process_foreign_fds we no longer can
> get FOREIGN_FD_XYZ flags set (the code only works now because
> FOREIGN_FD_READ & FOREIGN_FD_WRITE have the same values as
> TEVENT_FD_READ & TEVENT_FD_WRITE)

Yes, but afair process_foreign_fds() had no effect, you set
fde->additional_flags but the result wasn't used anywhere. But maybe I
missed something.

> Also 'run_events_poll besides' not
> being aware of the FOREIGN_FD_XYZ flags additionally doesn't monitor
> POLL or ERR flags (and glib for example at least in the api wants to
> have the possibility to register interest in those events) It's true
> that the example (as you have seen) and perhaps even libtracker-sparql
> in general doesn't need to know about POLL or ERR events (I wouldn't
> like to bet on that though)

Maybe it's time to add TEVENT_FD_HUP and TEVENT_FD_ERR to tevent. The
caller of tevent_add_fd() could signalize interest in these events and
tevent would pass the corresponding flag to the callback in case the
event (ie POLLHUP, POLLER) was triggered.

> But like I mentioned earlier I would like to see the relevant
> results from the poll function for foreign fds processed separately,
> ...

Hm, I see no need for that.

> ... I know the way I do it is inefficient (I iterate all fds to find
> the foreign ones) but it should be possible to do better and iterate
> only the foreign fds
> 
> [...]
> >
> >> e.g. in the s3
> >> tevent backend at least 'event_add_to_poll_args' and maybe even
> >> 'run_events_poll'. In anycase I would prefer to process the foreign
> >> events separately for 2 reasons and that activity also needs to be done
> >> inbetween the 'BEFORE' and 'AFTER' trace points. My reasons for
> >> processing *all* foreign fds at once are
> >>    a) I want the behaviour and results of the poll to be as close as
> >> possible to what the external code would expect if it polled the fds
> >> itself, that means if events for more than one fd are available I want
> >> them all and not one at a time
> >>    b) I think it could be wise to ensure the sequence (add foreign fds,
> >> poll foreign fds, report foreign fd poll results, react to foreign fds)
> >> happens all together without normal 'tevent' events happening inbetween
> > See attached patch, I think the glib loop is only entered once in
> > s3_tevent_foreign_loop_process().
> not sure what you mean, what I meant was I wanted to avoid the following
> scenario
> 
> prepare_poll (add some glib fds)
> poll  (some glib fds have events reported by poll)
> check_poll (is called but doesn't receive all the available events
> available from the glib fds because)
> 
>    a) some tevent has happened e.g. a normal fd monitored by tevent, a
> tevent timeout or a tevent signal
>    b) only a single glib fd event is available (because tevent loop only
> processes a single fd at a time) but the poll actually reported multiple
> glib fds have available events
> 
> Instead I wanted glib (or whatever foreign loop) to get the same results
> as if they called the poll function themselves. I know perhaps this is
> breaking the semantics of the tevent loop_once function but I look at
> this as something as 'outside' of tevent, I'm not really looking to
> impose tevent behaviour around polling foreign fds but rather just
> gather the results from whatever 'poll/select/epoll' function the 
> tevent backend is using and pass that on to whatever integration code is
> plugged in.

Not convinced. :) Both the tevent loop and the glib loop poll event
sources and run event handlers that must support being called in any
order. Adding tevent event handlers to the mix doesn't change the
requirements for glib event handlers.

> >  We use this in "rpc_daemon:mdssd =
> > fork, rpc_server:mdssvc = external" when running the RPC service in a
> > seperate process.
> >
> > With the current implementation we have one GMainContext per
> > connection, so the mdssd process running the RPC service is able to
> > isolate the client requests.
> >
> > In theory we could support multiple GMainContexts and maintain a list,
> > but then we'd poll all contexts at one in tevent and that won't work.
>
> hmm, I guess, I am not sure why you are using multiple GMainContexts but
> I suppose you had good reason and needed to do this to circumvent the
> problems of integrating with tevent in a 'custom' way, my gut feeling
> would be you don't need to do this

No, as said, I did this ...

> > so the mdssd process running the RPC service is able to
> > isolate the client requests.

-Ralph

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de,mailto:kontakt@sernet.de