Skip to content

dns: add dns_rr_dup and test_dns_rr_dup#1512

Merged
sreimers merged 1 commit intobaresip:mainfrom
maximilianfridrich:parallel_dns_fix
Feb 19, 2026
Merged

dns: add dns_rr_dup and test_dns_rr_dup#1512
sreimers merged 1 commit intobaresip:mainfrom
maximilianfridrich:parallel_dns_fix

Conversation

@maximilianfridrich
Copy link
Contributor

@maximilianfridrich maximilianfridrich commented Feb 11, 2026

No description provided.

@cspiel1
Copy link
Collaborator

cspiel1 commented Feb 12, 2026

Can confirm that two requests compete to have the same rr out of the DNS cache in their req->addrl. But still digging into the details what happens in the main thread when the DNS responses are handled nearly simultaneously.

test/dns.c Outdated
struct dns_query *q = NULL;
struct dns_query *q1 = NULL;
struct dns_query *q2 = NULL;
struct test_dns_cache_parallel parallel = {0};
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to move this to the regular tests ?

The regular tests include more variations like OOM, threading etc..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks for the feedback. We will keep working on this. We're pretty sure the shared pointers to the rr objects lead to the DNS problems we're experiencing with parallel outgoing SIP requests. We're trying to pinpoint it exactly and come up with a proper solution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The test does not check what currently is expected when dnsc API is used in request.c.

ASSERT_TRUE(parallel.rrv[0] != parallel.rrv[1]);

It checks that rr is not shared.

@cspiel1
Copy link
Collaborator

cspiel1 commented Feb 12, 2026

Currently we observe a failed DNS request if two SIP INVITE are tried to be sent directly one after another.

I am still not sure that there is a bug in client.c or request.c. Because the req->addrl is filled and consumed during one call of addr_handler(), making it possible to call two query handlers in a row that report the same rr.

Anyway, we have a failed SIP requests with addr_handler() returning EDESTADDRREQ with the parcall module. But only if empty AAAA responses are involved. Currently we are trying to understand, reproduce and fix what we observed at one of our customers.

@cspiel1
Copy link
Collaborator

cspiel1 commented Feb 12, 2026

Note, that q->rrlv[i] use rr->le_priv to link the elements, while req->addrl uses re->le. Thus it looks correct.

@cspiel1
Copy link
Collaborator

cspiel1 commented Feb 12, 2026

I think that I found the bug.
The empty AAAA response is not put into the DNS cache. Now suppose that two SIP requests are sent close enough before the first empty AAAA arrives. Both A queries can be found in the cash, but can't be processed immediately because of these check

	/* wait for other (A/AAAA) query to complete */
	if (req->dnsq || req->dnsq2)
		return;

The continuity of filling and consuming req->addlr is interrupted in this case. Only one of the requests will succeed.

I'll try to write a unit test that reproduces this.

@alfredh
Copy link
Contributor

alfredh commented Feb 13, 2026

This method is called Happy Eyeballs and can be used for e.g. SIP and HTTP, please take a look here:

https://datatracker.ietf.org/doc/html/rfc6555

Both DNS A+AAAA query is sent at the same time and the first one to respond will be used.
It may be used for a dualstack-host that has both IPv4+IPv6 address.

@cspiel1
Copy link
Collaborator

cspiel1 commented Feb 16, 2026

As a first step, should we add AAAA tests? #1515

Then we could add a test that runs A+AAAA query in parallel and reproduces the issue.

@maximilianfridrich maximilianfridrich changed the title dns: return independent RR objects for each cache-hit query dns: add dns_rr_dup and test_dns_rr_dup Feb 19, 2026
@maximilianfridrich maximilianfridrich marked this pull request as ready for review February 19, 2026 12:46
@cspiel1
Copy link
Collaborator

cspiel1 commented Feb 19, 2026

This looks good. The windows test run was already fixed. A rebase should solve this.

@sreimers sreimers merged commit f9e9bfe into baresip:main Feb 19, 2026
40 checks passed
@maximilianfridrich maximilianfridrich deleted the parallel_dns_fix branch February 19, 2026 15:01
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