Opened 14 months ago

Closed 8 months ago

#23750 closed enhancement (implemented)

Isolate libevent usage to a few locations

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: refactoring, technical-debt, review-group-31, 034-triage-20180328, 034-included-20180401
Cc: Actual Points:
Parent ID: #25500 Points:
Reviewer: dgoulet Sponsor: Sponsor8

Description

With #21841, we restricted openssl header usage to a small number of modules. We should do the same with libevent.

Child Tickets

Change History (10)

comment:1 Changed 14 months ago by nickm

I've started some work here as isolate_libevent. Let's take a look and maybe merge it when 0.3.3 rolls around.

comment:2 Changed 10 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final
Status: assignedneeds_review

comment:3 Changed 10 months ago by nickm

Keywords: review-group-31 added

comment:4 Changed 10 months ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewneeds_revision

From commit 6c5a4bef4e77cf37:

  • I would propose that mainloop_event_new() validates at least the cb argument passed with a tor_assert() (seems it can't be NULL nor it makes sense to be NULL).

I would also document the function that the userdata ownership is passed to the returned mainloop_event_.

  • Can we use mainloop_event_activate() if the event was never scheduled/added to the main loop?
  • Can I use tv = NULL in mainloop_event_schedule() to tell it "now" or tv must be a valid pointer?
  • I would document what this does if called multiple time: mainloop_event_cancel().
  • Maybe we should have this using the FREE_AND_NULL macro scheme? mainloop_event_free()

From commit 3edf64a95f91d9be:

  • This function threadpool_register_reply_event() doesn't use the nice mainloop_event_t API and I can see that is because we create an event with specific flags (EV_PERSIST | EV_READ).


And then it goes on using event_add() directly. Couldn't we remove all this and make it that it would use that mainloop interface created prior?

  • Also, why is threadpool_register_reply_event() is taking a base event, can't we abstract that to always use tor_libevent_get_base() or that doesn't play nice in multi thread?

comment:5 Changed 8 months ago by nickm

Keywords: 034-triage-20180328 added

comment:6 Changed 8 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:7 Changed 8 months ago by nickm

Keywords: 034-included-20180401 added; 034-removed-20180328 removed
Parent ID: #25500
Sponsor: Sponsor8

This is part of #25500, since it will help with some of our other mainloop improvements.

comment:8 Changed 8 months ago by nickm

Status: needs_revisionneeds_review

Okay, I've made the requested changes!

First I had to rebase the branch, since there were several incompatible changes to master since this branch began. Now it is in a new branch called isolate_libevent_2.

I made the changes you requested, except for this:

This function threadpool_register_reply_event() doesn't use the nice mainloop_event_t API

The problem here is that mainloop_event_t is only for timed events or events that we trigger via event_active(): we can't use it for events that trigger via a socket.

comment:9 Changed 8 months ago by dgoulet

Status: needs_reviewmerge_ready

Thanks. Changes lgtm! Not sure if it passes the CI but it passes make test I'm happy with the fixes.

comment:10 Changed 8 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Travis seems okay with it. Squashed and merged!

Note: See TracTickets for help on using tickets.