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.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.