> On Feb. 6, 2014, 2:21 p.m., rmudgett wrote:
> > branches/12/main/sorcery.c, lines 1630-1633
> > <https://reviewboard.asterisk.org/r/3184/diff/3/?file=53617#file53617line1630>
> >
> > There are two red flags here:
> > 1) You are using sorcery after you have released your ref to it.
> >
> > 2) Any time you are looking at the number of references left, there is
> > a design problem. In this case, only the owner of the sorcery object
> > should close/destroy it which will remove it from the instances container.
> > Only the owner of the sorcery object should open/create it which will add
> > it to the instances container. Everyone else will search the instances
> > container for the object to get a reference.
>
> George Joseph wrote:
> There's no concept of weak references so when the sorcery instance is
> linked to the registry container, the refcount gets bumped without the actual
> consumer knowing. When the last consumer releases the instance, the
> instance's refcount is still 1 so the destructor will never run and and
> remove the entry. Instead of checking the refcount I could simply decrement
> it automatically after inserting the instance to the container. This way the
> last consumer to release it will trigger the cleanup.
>
> Now, if you're saying that only 1 consumer should ever call
> ast_sorcery_open then I need to rethink. The original idea was that multiple
> consumers could simply call ast_sorcery_open and let it figure out if it
> needed to create a new one or return the existing one.
>
>
>
> rmudgett wrote:
> You cannot decrement the ref that is held by the container without
> causing problems for the container when it decrements the the ref on an
> unlink. You would get a negative ref count.
>
> I was thinking only one creator/owner should ever call the open/destroy.
>
> Bridges and channels have the same kind of problem since they are also
> put into a global container. They have trigger events to know when they need
> to be destroyed so they can unlink from the global container.
>
> George Joseph wrote:
> Yeah, -1 to the auto -1. :)
>
> So I'd have to create a balancing ast_sorcery_close() that did a complete
> clean of the sorcery instance. Should it clean and destroy (possibly pulling
> it out from under someone who did a retrieve on it but never -1'd it) or just
> remove it from the registry and let the last -1 clean it up?
>
> What should happen if the same module (hence the same key) tries to open
> twice?
>
> BTW, the sorcery instance opened by res_pjsip/config_system gets passed
> all over pjsip-land, even to other modules, without anyone ever doing a +1 on
> it. I'd want to go clean those up as well but don't think it should be
> part of this patch.
>
>
> Joshua Colp wrote:
> This may be a time where it is near impossible to do this in a clean
> fashion that is safe...
If that's the case then the following will have to do:
ast_sorcery_unref(sorcery)
{
if (sorcery) {
/* One ref for what we just released, the other for the instances
container. */
if (ao2_ref(sorcery, -1) == 2) {
ao2_unlink(instances, sorcery);
}
}
}
- rmudgett
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3184/#review10799
-----------------------------------------------------------
On Feb. 6, 2014, 2:07 p.m., George Joseph wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3184/
> -----------------------------------------------------------
>
> (Updated Feb. 6, 2014, 2:07 p.m.)
>
>
> Review request for Asterisk Developers.
>
>
> Bugs: ASTERISK-22537
> https://issues.asterisk.org/jira/browse/ASTERISK-22537
>
>
> Repository: Asterisk
>
>
> Description
> -------
>
> Create sorcery instance registry as a precursor to creating a generic
> dialplan function that can retrieve parameters from a sorcery-based config
> file.
>
> ast_sorcery_init now creates a hashtab as a registry.
> ast_sorcery_open now checks the hashtab for an existing sorcery instance
> matching the caller's module name. If it finds one, it bumps the refcount
> and returns it. If not, it creates a new sorcery instance, adds it to the
> hashtab, then returns it.
> ast_sorcery_retrieve_by_module_name is a new function that does a hashtab
> lookup by module name. It can be called by the future dialplan function.
>
> A side effect of this patch is that a module can only have 1 sorcery instance
> (because it's the key for the hashtab). res_pjsip/config_system needed a
> small change to share the main res_pjsip sorcery instance.
>
>
> Diffs
> -----
>
> branches/12/tests/test_sorcery.c 407566
> branches/12/res/res_pjsip/include/res_pjsip_private.h 407566
> branches/12/res/res_pjsip/config_system.c 407566
> branches/12/res/res_pjsip.c 407566
> branches/12/main/sorcery.c 407566
> branches/12/include/asterisk/sorcery.h 407566
>
> Diff: https://reviewboard.asterisk.org/r/3184/diff/
>
>
> Testing
> -------
>
> Made sure that users of sorcery (mostly res_pjsip) continued to load their
> configs correctly.
> Made sure there were no ill effects on res_pjsip from config_system sharing
> the same sorcery instance as the rest of the pjsip infrastructure.
> Made sure that config_system was properly marked as 'not reloadable' and that
> it was maintaining it's original values when res_pjsip was reloaded.
>
>
> ernie*CLI> test execute category /main/sorcery/
> Running all available tests matching category /main/sorcery/
>
> START /main/sorcery/ - configuration_file_wizard_retrieve_multiple_all
> END /main/sorcery/ - configuration_file_wizard_retrieve_multiple_all Time:
> <1ms Result: PASS
> [snip]
> START /main/sorcery/ - open
> END /main/sorcery/ - open Time: <1ms Result: PASS
> START /main/sorcery/ - wizard_registration
> END /main/sorcery/ - wizard_registration Time: <1ms Result: PASS
>
> 43 Test(s) Executed 43 Passed 0 Failed
>
>
> Thanks,
>
> George Joseph
>
>
--
_____________________________________________________________________
-- 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
