Skip to content

test: dns - parallel queries#1520

Closed
cspiel1 wants to merge 6 commits intobaresip:mainfrom
cspiel1:test_dns_parallel
Closed

test: dns - parallel queries#1520
cspiel1 wants to merge 6 commits intobaresip:mainfrom
cspiel1:test_dns_parallel

Conversation

@cspiel1
Copy link
Collaborator

@cspiel1 cspiel1 commented Feb 17, 2026

  • test: dns - parallel queries
  • test: dns - parallel use of RR
  • test: dns - fix parallel use of rr

There are situations where multiple references of the same rr object are used at different places. E.g. in two SIP requests that are sent in parallel.

This PR adds a test of this use case.

Another PR that fixes request.c will follow.
Then dnsrr->le becomes obsolete and can be removed.

@cspiel1 cspiel1 marked this pull request as draft February 17, 2026 14:45
@cspiel1 cspiel1 marked this pull request as ready for review February 18, 2026 09:20
Comment on lines +351 to +355
struct le *le = mem_zalloc(sizeof(*le), NULL);
if (!le)
return true;

list_append(lst, le, mem_ref(rr));
Copy link
Collaborator Author

@cspiel1 cspiel1 Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be added to a new list function. E.g. list_append_ref(lst, data).
The struct le could get a flag to know if list_flush() should also deref the le.

Edit: What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First let me know what you think about this: #1527

For now I set this to draft.

@cspiel1 cspiel1 marked this pull request as draft February 18, 2026 14:36
@alfredh
Copy link
Contributor

alfredh commented Feb 19, 2026

The struct le le element is supposed to be a compound member of a struct, so that when the
struct is deleted, the le element is also deleted.

While the code is inside the DNS callback handler, the RR entries should be valid.
If the application needs to save those RR entries after the callback has returned,
it should copy them.

Perhaps it would be a solution to clone the RR entry ?

@sreimers
Copy link
Member

Yes maybe a:

struct dnsrr *rrdup = dns_rr_dup(rr);
...
list_append(lst, &rrdup->le, rrdup);

would be much nicer.

@cspiel1
Copy link
Collaborator Author

cspiel1 commented Feb 19, 2026

Oh, there happened something in parallel. @maximilianfridrich and me discussed currently and would suggest this solution: #1525

In general this should be possible

list_append(lst, le, mem_ref(data));

But the user of list_append() needs to alloc the le and implement its own flush code. There is no need to implement a deep copy.

Edit: Anyway, if you prefer the deep copy. We could proceed here: #1512. Just let us know!

@sreimers
Copy link
Member

sreimers commented Feb 19, 2026

As mentioned by @alfredh a struct le should be part of another struct. So yes I think the copy is a better and a more readable solution, too. So the fix should not be within dns client, the assumption that sip/request.c can take over the list rr ownership after the callback is executed is wrong here IMHO.

So this could be the fix:

--- a/src/sip/request.c
+++ b/src/sip/request.c
@@ -384,7 +384,8 @@ static bool rr_append_handler(struct dnsrr *rr, void *arg)
                if (rr->le.list)
                        break;
 
-               list_append(lst, &rr->le, mem_ref(rr));
+               struct dnsrr *rrdup = dns_rr_dup(rr);
+               list_append(lst, &rrdup->le, rrdup);
                break;
        }

Or instead of a deep rr copy, maybe more simple address list handling could be used, like on other places:

static bool rr_handler(struct dnsrr *rr, void *arg)
{
	struct http_req *req = arg;

	if (req->srvc >= RE_ARRAY_SIZE(req->srvv))
		return true;

	switch (rr->type) {

	case DNS_TYPE_A:
		sa_set_in(&req->srvv[req->srvc++], rr->rdata.a.addr,
			  req->port);
		break;

	case DNS_TYPE_AAAA:
		sa_set_in6(&req->srvv[req->srvc++], rr->rdata.aaaa.addr,
			   req->port);
		break;
	}

	return false;
}

@cspiel1
Copy link
Collaborator Author

cspiel1 commented Feb 19, 2026

If a simpler rr address list handling is used, then we have to replace also this list sort

			dns_rrlist_sort_addr(&req->addrl, req->sortkey);

@maximilianfridrich
Copy link
Contributor

Thanks for the reviews and feedback! Since the duplication/copying solution is preferred, I implemented the dns_rr_dup function here #1512 and will then create a new PR for sip/request.c where the cloning function will be used.

@cspiel1 cspiel1 closed this Feb 19, 2026
@cspiel1 cspiel1 deleted the test_dns_parallel branch February 19, 2026 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments