On Thu, Feb 09, 2023 at 07:58:55PM +0100, Omar Polo wrote:
> On 9 February 2023 19:10:19 CET, Stefan Sperling <stsp@stsp.name> wrote:
> >On Thu, Feb 09, 2023 at 06:58:14PM +0100, Omar Polo wrote:
> >> On 2023/02/09 17:19:25 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> >> > Not a functional change. But while trying to figure out why gotd
> >> > was crashing in libevent, this stood out as a potential suspect
> >> > of libevent misuse.
> >> 
> >> it took me a bit to see that this is safe.  It will still delete the
> >> event in disconnect().
> >> 
> >> it's certainly better than calling event_del in multiple places, but
> >> this also means that we could be awakened to dispatch on gotd_request
> >> multiple times for events that we're not interested in.
> >
> >We should at most only wake up once because our events are not marked
> >with EV_PERSIST. The basic idea of this change is to skip the
> >gotd_imsg_event_add() call at the very bottom of gotd_request().
> >If that function is not called we should never be notified for
> >activity on the client's socket again.
> 
> ah, right, yes
> 
> >During early development of gotd this has bitten me a bunch of times,
> >where everything appeared to stall because I essentially forgot to
> >call event_add() again.
> >
> >> maybe we
> >> could just re-initialize the event in connect_session?  event_del +
> >> event_set (without event_add) should do it.
> >
> >I don't know if doing this is safe while we are already being called
> >by libevent to handle another event.
> 
> it's exactly what gotd_imsg_event_add does to reschedule the event IIRC.  but agre that it's not a clean solution.
> 
> > It may well be the case. But to
> >me it seems safer to "disable" an event by simply not adding it again
> >after it fires.
> 
> hmm. I still think thank this is just trying to avoid the issue.  there's a reason I'm not seeing we can't use client->fd = -1 as a flag to indicate whether the event is live and skip event_del in disconnect?
> 
> event_del there next to msgbuf_drain seems the right place to me to, would actually seem strange to see the drain without the removal of the event.
 
I don't think calling event_del() multiple times is a problem.
If an event has not been added or is not queued on any internal
lists, libevent will do nothing when we try to delete the event.

But let's skip this diff for now. It is not addressing an actual
problem, and the existing code should work as-is. So unless we
discover a bug that can really be pinned down to this code path,
there is no real need to change anything. You are making a good
point saying that we should stick to the idiom of draining pending
outgoing messages and removing the incoming trigger at the same time.