You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@qpid.apache.org by Jiri Daněk <jd...@redhat.com> on 2021/07/16 09:54:52 UTC
[Dispatch] what's the purpose of `type_registered` flag in router_node.c
Hello folks
```
static int type_registered = 0;
qd_router_t *qd_router(qd_dispatch_t *qd, qd_router_mode_t mode, const char
*area, const char *id)
{
if (!type_registered) {
type_registered = 1;
qd_container_register_node_type(qd, &router_node);
}
```
https://github.com/apache/qpid-dispatch/blob/d8800269d3a80225794be9914b5fc9f8d6118d04/src/router_node.c#L1623-L1630
I'd like to understand the motivation behind this code better.
Some parts of the codebase assume that there may be many qd_dispatch_t
instances around, while others assume there is only one. There is the
`dispatch` global variable in python_embedded.c, there is the global flag
`type_registered` here, but the `qd_dispatch_t` pointer is usually passed
through function argument (as if there could be more than one).
Having this check for `type_registered` prevents me from stopping and
freeing one instance of a router, then immediately starting another in the
same process. I want to do this for testing purposes. What happens now is
that the second router I start will not function correctly; deleting this
type_registered logic makes it work right (as far as my tests so far are
concerned).
It seems to me that it should be perfectly ok to have multiple dispatch
instances in the single process, as long as there is only one at a time.
--
Mit freundlichen Grüßen / Kind regards
Jiri Daněk
Re: [Dispatch] what's the purpose of `type_registered` flag in router_node.c
Posted by Ted Ross <te...@nethopper.io>.
On Fri, Jul 16, 2021 at 10:36 AM Ken Giusti <kg...@redhat.com> wrote:
> On Fri, Jul 16, 2021 at 9:39 AM Ted Ross <te...@nethopper.io> wrote:
>
> > Hi Jiri,
> >
> > In normal operation of the router, there is only one call to 'qd_router',
> > so I'm not sure we really need 'type_registered'. That code has been
> there
> > from the very beginning so I don't think it was added to fix a particular
> > problem. I think it could safely be removed.
> >
> >
> Hey Ted - do you think there'd be a benefit in terms of reduced code if we
> refactor out the old generic node-based dispatcher layer?
>
Probably. There's been talk in the past about eliminating the "container"
layer altogether thus removing one level of callbacks. I think the right
approach would be to refactor the Proton interface to be a protocol
adaptor. This would have the added benefit of allowing management hooks
for AMQP access via Skupper.
But, like you said, probably not a high priority.
>
> Not a high priority of course, but if there's ways to simplify the codebase
> that would be great.
>
> -K
>
>
>
> > -Ted
> >
> > On Fri, Jul 16, 2021 at 5:56 AM Jiri Daněk <jd...@redhat.com> wrote:
> >
> > > Hello folks
> > >
> > > ```
> > > static int type_registered = 0;
> > >
> > > qd_router_t *qd_router(qd_dispatch_t *qd, qd_router_mode_t mode, const
> > char
> > > *area, const char *id)
> > > {
> > > if (!type_registered) {
> > > type_registered = 1;
> > > qd_container_register_node_type(qd, &router_node);
> > > }
> > > ```
> > >
> > >
> >
> https://github.com/apache/qpid-dispatch/blob/d8800269d3a80225794be9914b5fc9f8d6118d04/src/router_node.c#L1623-L1630
> > >
> > > I'd like to understand the motivation behind this code better.
> > >
> > > Some parts of the codebase assume that there may be many qd_dispatch_t
> > > instances around, while others assume there is only one. There is the
> > > `dispatch` global variable in python_embedded.c, there is the global
> flag
> > > `type_registered` here, but the `qd_dispatch_t` pointer is usually
> passed
> > > through function argument (as if there could be more than one).
> > >
> > > Having this check for `type_registered` prevents me from stopping and
> > > freeing one instance of a router, then immediately starting another in
> > the
> > > same process. I want to do this for testing purposes. What happens now
> is
> > > that the second router I start will not function correctly; deleting
> this
> > > type_registered logic makes it work right (as far as my tests so far
> are
> > > concerned).
> > >
> > > It seems to me that it should be perfectly ok to have multiple dispatch
> > > instances in the single process, as long as there is only one at a
> time.
> > > --
> > > Mit freundlichen Grüßen / Kind regards
> > > Jiri Daněk
> > >
> >
>
>
> --
> -K
>
Re: [Dispatch] what's the purpose of `type_registered` flag in router_node.c
Posted by Ken Giusti <kg...@redhat.com>.
On Fri, Jul 16, 2021 at 9:39 AM Ted Ross <te...@nethopper.io> wrote:
> Hi Jiri,
>
> In normal operation of the router, there is only one call to 'qd_router',
> so I'm not sure we really need 'type_registered'. That code has been there
> from the very beginning so I don't think it was added to fix a particular
> problem. I think it could safely be removed.
>
>
Hey Ted - do you think there'd be a benefit in terms of reduced code if we
refactor out the old generic node-based dispatcher layer?
Not a high priority of course, but if there's ways to simplify the codebase
that would be great.
-K
> -Ted
>
> On Fri, Jul 16, 2021 at 5:56 AM Jiri Daněk <jd...@redhat.com> wrote:
>
> > Hello folks
> >
> > ```
> > static int type_registered = 0;
> >
> > qd_router_t *qd_router(qd_dispatch_t *qd, qd_router_mode_t mode, const
> char
> > *area, const char *id)
> > {
> > if (!type_registered) {
> > type_registered = 1;
> > qd_container_register_node_type(qd, &router_node);
> > }
> > ```
> >
> >
> https://github.com/apache/qpid-dispatch/blob/d8800269d3a80225794be9914b5fc9f8d6118d04/src/router_node.c#L1623-L1630
> >
> > I'd like to understand the motivation behind this code better.
> >
> > Some parts of the codebase assume that there may be many qd_dispatch_t
> > instances around, while others assume there is only one. There is the
> > `dispatch` global variable in python_embedded.c, there is the global flag
> > `type_registered` here, but the `qd_dispatch_t` pointer is usually passed
> > through function argument (as if there could be more than one).
> >
> > Having this check for `type_registered` prevents me from stopping and
> > freeing one instance of a router, then immediately starting another in
> the
> > same process. I want to do this for testing purposes. What happens now is
> > that the second router I start will not function correctly; deleting this
> > type_registered logic makes it work right (as far as my tests so far are
> > concerned).
> >
> > It seems to me that it should be perfectly ok to have multiple dispatch
> > instances in the single process, as long as there is only one at a time.
> > --
> > Mit freundlichen Grüßen / Kind regards
> > Jiri Daněk
> >
>
--
-K
Re: [Dispatch] what's the purpose of `type_registered` flag in router_node.c
Posted by Jiri Daněk <jd...@redhat.com>.
Thanks Ted, this really helps! Linking
https://issues.apache.org/jira/browse/DISPATCH-2201 to
track type_registered's removal.
On Fri, Jul 16, 2021 at 3:40 PM Ted Ross <te...@nethopper.io> wrote:
> Hi Jiri,
>
> In normal operation of the router, there is only one call to 'qd_router',
> so I'm not sure we really need 'type_registered'. That code has been there
> from the very beginning so I don't think it was added to fix a particular
> problem. I think it could safely be removed.
>
> -Ted
>
> On Fri, Jul 16, 2021 at 5:56 AM Jiri Daněk <jd...@redhat.com> wrote:
>
> > Hello folks
> >
> > ```
> > static int type_registered = 0;
> >
> > qd_router_t *qd_router(qd_dispatch_t *qd, qd_router_mode_t mode, const
> char
> > *area, const char *id)
> > {
> > if (!type_registered) {
> > type_registered = 1;
> > qd_container_register_node_type(qd, &router_node);
> > }
> > ```
> >
> >
> https://github.com/apache/qpid-dispatch/blob/d8800269d3a80225794be9914b5fc9f8d6118d04/src/router_node.c#L1623-L1630
> >
> > I'd like to understand the motivation behind this code better.
> >
> > Some parts of the codebase assume that there may be many qd_dispatch_t
> > instances around, while others assume there is only one. There is the
> > `dispatch` global variable in python_embedded.c, there is the global flag
> > `type_registered` here, but the `qd_dispatch_t` pointer is usually passed
> > through function argument (as if there could be more than one).
> >
> > Having this check for `type_registered` prevents me from stopping and
> > freeing one instance of a router, then immediately starting another in
> the
> > same process. I want to do this for testing purposes. What happens now is
> > that the second router I start will not function correctly; deleting this
> > type_registered logic makes it work right (as far as my tests so far are
> > concerned).
> >
> > It seems to me that it should be perfectly ok to have multiple dispatch
> > instances in the single process, as long as there is only one at a time.
> > --
> > Mit freundlichen Grüßen / Kind regards
> > Jiri Daněk
> >
>
--
Mit freundlichen Grüßen / Kind regards
Jiri Daněk
Re: [Dispatch] what's the purpose of `type_registered` flag in router_node.c
Posted by Ted Ross <te...@nethopper.io>.
Hi Jiri,
In normal operation of the router, there is only one call to 'qd_router',
so I'm not sure we really need 'type_registered'. That code has been there
from the very beginning so I don't think it was added to fix a particular
problem. I think it could safely be removed.
-Ted
On Fri, Jul 16, 2021 at 5:56 AM Jiri Daněk <jd...@redhat.com> wrote:
> Hello folks
>
> ```
> static int type_registered = 0;
>
> qd_router_t *qd_router(qd_dispatch_t *qd, qd_router_mode_t mode, const char
> *area, const char *id)
> {
> if (!type_registered) {
> type_registered = 1;
> qd_container_register_node_type(qd, &router_node);
> }
> ```
>
> https://github.com/apache/qpid-dispatch/blob/d8800269d3a80225794be9914b5fc9f8d6118d04/src/router_node.c#L1623-L1630
>
> I'd like to understand the motivation behind this code better.
>
> Some parts of the codebase assume that there may be many qd_dispatch_t
> instances around, while others assume there is only one. There is the
> `dispatch` global variable in python_embedded.c, there is the global flag
> `type_registered` here, but the `qd_dispatch_t` pointer is usually passed
> through function argument (as if there could be more than one).
>
> Having this check for `type_registered` prevents me from stopping and
> freeing one instance of a router, then immediately starting another in the
> same process. I want to do this for testing purposes. What happens now is
> that the second router I start will not function correctly; deleting this
> type_registered logic makes it work right (as far as my tests so far are
> concerned).
>
> It seems to me that it should be perfectly ok to have multiple dispatch
> instances in the single process, as long as there is only one at a time.
> --
> Mit freundlichen Grüßen / Kind regards
> Jiri Daněk
>