> On March 20, 2014, 5:12 a.m., Corey Farrell wrote:
> > We should remove utils/refcounter from all branches. The format is
> > changing, so the existing utils/refcounter will be unusable with 1.8.27.0 /
> > 11.9.0 / 12.2.0. I think providing utils/refcounter in those versions will
> > only cause confusion. Maybe we could enable installation of refcounter.py
> > to ${ASTSBINDIR}/refcounter?
>
> Matt Jordan wrote:
> I disagree on removing refcounter, for the reasons Richard gave. Removing
> it is liable to result in more confusion, since its absence will be more
> noticeable than its presence.
>
> I'm not sure about installing refcounter.py by default. You would only
> run it when things have gone wrong and you need extra debugging. That feels
> like something that should be in the scripts folder, and not always available.
Actually I think Richard was saying to only keep it in cleanup, to ensure that
the stale utils/refcounter is deleted. I'm fine with not installing
refcounter.py, but utils/refcounter will not work with the new ref_log format,
so I think it should go away.
> On March 20, 2014, 5:12 a.m., Corey Farrell wrote:
> > /branches/1.8/main/astobj2.c, lines 228-233
> > <https://reviewboard.asterisk.org/r/3377/diff/2/?file=56243#file56243line228>
> >
> > Can we deduplicate the ref_log writes into a macro? I'm suggesting a
> > macro so we can pass "%d" or "%s" for the format of the 'state' field, and
> > let the compiler join the const strings.
> >
> > This would make it easier to append additional fields if desired, and
> > ensure consistent output. For example it is sometimes useful to add the
> > current thread id to all ref_log messages.
>
> Matt Jordan wrote:
> I'm not sure how a macro is going to simplify this here. The state field
> in particular seems like it would necessitate at least two macros; at the end
> of the day, something has to provide a format string ("%d" or "%s"), and
> having two macros for three logging statements feels like overkill.
> On March 20, 2014, 5:12 a.m., Corey Farrell wrote:
> > /branches/1.8/main/astobj2.c, lines 1146-1152
> > <https://reviewboard.asterisk.org/r/3377/diff/2/?file=56243#file56243line1146>
> >
> > What about ao2 objects created before astobj2_init? I know that
> > storing refs in astlogdir was my idea/request, but I'm concerned this will
> > cause an unfixable issue with initialization order. If we open the file
> > too early, ast_config_AST_LOG_DIR will be the compiled value instead of the
> > value in asterisk.conf. If we open too late, startup messages are lost.
> > Further it's impossible to read astlogdir from asterisk.conf before
> > __attribute__((constructor)).
> >
> > What if we allow an environmental variable to override /tmp/refs? My
> > goal with using astlogdir was to allow multiple instances of asterisk to
> > run without causing conflicts in /tmp/refs. This would be essential for
> > the testsuite to check refs after tests.
> >
> > I don't think we can (safely) avoid opening/closing the refs file for
> > every output. Doing so would require us to open ref_log before any other
> > __attribute__((constructor)) and closing it after all other
> > __attribute__((destructor)), I think this would be impossible without
> > starting to use constructor/destructor priorities.
>
> Matt Jordan wrote:
> If you create an ao2 object before astobj2_init, then you have broken the
> module load order. You should *not* ever create an ao2 object before that
> point.
>
> This may mean that astobj2_init has to move higher in the module load
> order. I've gone ahead and moved it up to being initialized immediately after
> reading the asterisk.conf file, as the CLI is initialized by that point as
> well. I'd rather not try to work around things that violate the order: if you
> try to access something in the core prior to it being initialized, that's an
> error.
>
> Note that astlogdir is read from asterisk.conf via read_config_maps. This
> happens immediately after printing out the intro message and registering the
> basic CLI functions. Again, if you create an ao2 object prior to reading
> asterisk.conf, you're violating the initialization order of the core and
> that's an error. Don't do that. :-)
>
> An environment variable that overrides the refs file is fine, but I'd
> consider that a separate feature from this patch.
>
> I don't think we need to open/close the refs file. That still feels like
> overkill, as fflush will write all data to disk. As astobj2 is initialized
> prior to any modules being loaded into memory, the ref_log file will be
> created before any constructor/destructor calls - which typically do not
> create ao2 objects as well (as they are used to set up module information).
>
What about AO2_GLOBAL_OBJ_STATIC? I see now that it's not ao2_alloc'ed during
__attribute__((constructor)), but it can be unref'ed during
__attribute__((destructor)). You're probably right about the initialization
order. Your initialization reorder should be good so long as config.c doesn't
start using ao2, but I think my concern still applies to shutdown order.
- Corey
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3377/#review11290
-----------------------------------------------------------
On March 27, 2014, 5:08 p.m., Matt Jordan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3377/
> -----------------------------------------------------------
>
> (Updated March 27, 2014, 5:08 p.m.)
>
>
> Review request for Asterisk Developers, Corey Farrell and wdoekes.
>
>
> Repository: Asterisk
>
>
> Description
> -------
>
> Note: while an improvement to Asterisk, this patch only affects Asterisk when
> compiled in -dev-mode. Since it has benefit only for developers looking to
> fix bugs, I'm proposing it for Asterisk 1.8+. The removal of refcounter
> should be done in trunk only.
>
> Asterisk uses reference counted objects. A lot. As their use has spread, the
> bugs related to reference counting errors has grown as well. Central to
> resolving reference counting issues is the usage of REF_DEBUG; unfortunately,
> REF_DEBUG has had several problems:
> (1) It could not be enabled through menuselect
> (2) When not enabled globally, updates to objects were often lost, resulting
> in insufficient data to resolve problems
> (3) The format of the ref debug log file was not suitable for parsing
>
> This patch changes REF_DEBUG in the following ways:
> (1) It makes REF_DEBUG a meneselect item when Asterisk is compiled with
> --enable-dev-mode
> (2) The ref debug log file is now created in the AST_LOG_DIR directory. Every
> run will now blow away the previous run (as large ref files sometimes caused
> issues). We now also no longer open/close the file on each write, instead
> relying on fflush to make sure data gets written to the file (in case the ao2
> call being performed is about to cause a crash)
> (3) It goes with a comma delineated format for the ref debug file. This makes
> parsing much easier.
> (4) It throws out the refcounter utility and goes with a python script
> instead.
>
>
> Diffs
> -----
>
> /branches/1.8/utils/refcounter.c 410891
> /branches/1.8/utils/Makefile 410891
> /branches/1.8/main/astobj2.c 410891
> /branches/1.8/main/asterisk.c 410891
> /branches/1.8/include/asterisk/astobj2.h 410891
> /branches/1.8/contrib/scripts/refcounter.py PRE-CREATION
> /branches/1.8/channels/chan_sip.c 410891
> /branches/1.8/build_tools/cflags.xml 410891
>
> Diff: https://reviewboard.asterisk.org/r/3377/diff/
>
>
> Testing
> -------
>
> Things spit out ref logs and the script parses them. Example below:
>
> ==== Object 0x21ed9b8 history ====
> features.c[5427]:create_parkinglot 1 - [**constructor**]
> features.c[5707]:build_parkinglot +1 - [1]
> features.c[5392]:parkinglot_unref -1 - [2]
> features.c[6358]:build_dialplan_useage_map +1 - [1]
> features.c[6358]:build_dialplan_useage_map -1 - [2]
> features.c[4985]:find_parkinglot +1 - [1]
> features.c[5392]:parkinglot_unref -1 - [2]
> features.c[6358]:build_dialplan_useage_map +1 - [1]
> features.c[6358]:build_dialplan_useage_map -1 - [2]
> features.c[4955]:do_parking_thread +1 - [1]
> features.c[4957]:do_parking_thread -1 - [2]
> astobj2.c[955]:cd_cb_debug -1 deref object via container destroy - [1]
> astobj2.c[955]:cd_cb_debug -1 deref object via container destroy - [**call
> destructor**]
>
>
> Thanks,
>
> Matt Jordan
>
>
--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
