From: Omar Polo Subject: Re: small gotd event_del() tweak To: Stefan Sperling Cc: gameoftrees@openbsd.org Date: Thu, 09 Feb 2023 19:58:55 +0100 On 9 February 2023 19:10:19 CET, Stefan Sperling wrote: >On Thu, Feb 09, 2023 at 06:58:14PM +0100, Omar Polo wrote: >> On 2023/02/09 17:19:25 +0100, Stefan Sperling 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. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.