test addr/parse takes a long time on master on some machines

A recent commit to master causes the addr/parse test to run for a few minutes on my Debian 10.1 install. The test works fine on maint-0.4.1, and on some of my older master branches.

This issue doesn't seem to affect our CI builders.

I haven't had time to bisect yet.

changed milestone to %Tor: 0.4.2.x-final

added 042-should BugSmashFund actualpoints::.2 component::core tor/tor milestone::Tor: 0.4.2.x-final owner::nickm priority::high resolution::fixed reviewer::teor severity::normal status::closed tor-unit-tests type::defect version::tor unspecified labels

Trac:
Description: A recent commit to master causes the addr/parse test to hang on my Debian 10.1 install. The test works fine on maint-0.4.1, and on some of my older master branches.

This issue doesn't seem to affect our CI builders.

I haven't had time to bisect yet.

to

A recent commit to master causes the addr/parse test to run for a few minutes on my Debian 10.1 install. The test works fine on maint-0.4.1, and on some of my older master branches.

This issue doesn't seem to affect our CI builders.

I haven't had time to bisect yet.
Summary: test addr/parse hangs on master on some machines to test addr/parse takes a long time on master on some machines

Trac:
Priority: Medium to High
Keywords: tor-unit-tests deleted, tor-unit-tests 042-must added

This seems to be a network dependency:

• when the network interface is up, sometimes the tests are fast
• when the network interface is down, the tests are almost always slow

Because it's intermittent, I might be wrong about this bug being new in master.

I wonder if this is a hostname-lookup issue.

I think the solution here is, if the test does any hostname lookups, to mock out the lowest-level getaddrinfo or gethostbyname function, so that we aren't going out to the network from this test.

Trac:
Keywords: tor-unit-tests 042-must deleted, tor-unit-tests 042-should added

Trac:
Owner: N/A to nickm
Status: new to accepted

I think it's not just one test, these other tests also seem to be affected:

• test/options_*

Maybe we should mock those functions for all unit tests?

In 0.4.3, we should consider failing tests that take longer than N seconds. I'll open another ticket.

I suspected systems with slow DNS. Fortunately, all our blocking DNS lookup happens in one place, in one block within tor_addr_lookup(), so it's easy to see which unit tests call it. They are:

• addr/parse (over 180 times)
• config/parse_port_config__ports__ports_given (twice)
• hs_config/valid_service_v2 (once)
• hs_config/valid_service_v3 (once)
• options/validate (once)

So if a system has a DNS resolver that is susceptible to timeouts, we would expect addr/parse to block, and nothing else.

(Of course, if the callback is really slow, the other tests might block too.)

I opened #31940 (moved) for follow-up.

Let's decide if we want to do it in 0.4.3, based on how well this fix works.

Branch is ticket31841; PR at https://github.com/torproject/tor/pull/1393

I suspect that coverage may go down, since we are removing all the code that used to call into tor_addr_lookup_host_{getaddrinfo,gethostbyname}() no longer does so. I'm not sure of a meaningful way to test those, however, without potentially invoking a DNS resolver.

I'll put this in needs_review once CI has passed.

Trac:
Keywords: tor-unit-tests 042-should deleted, tor-unit-tests 042-should BugSmashFund added
Actualpoints: N/A to .2

Needs_review. I think that most of the lost coverage outside of lib/net/resolve.c is spurious, but I can investigate more if needed.

Trac:
Status: accepted to needs_review

I left a few minor comments on the pull request, feel free to rebase and edit existing commits to fix them.

I checked the coverage loss, it did seem spurious.

Do we need to backport this change to 0.2.9 and later? We're going to be running tests on 0.2.9 for the next 3 months, and 0.3,5 for the next 2 years.

Trac:
Status: needs_review to needs_revision

I'd suggest that we not backport. To me, this seems more like a feature than a bug.

I've turned the printf() into a debug log, and rebased the branch to move the mocking into the proper commit.

Trac:
Status: needs_revision to needs_review

"the tests won't hang if your DNS is slow" doesn't really seem like a feature. Particularly when I'm the tester, and my builds are incredibly slow.

Let's check how far back the 180 addr/parse calls go? I think it's 0.4.2, but it might be 0.4.1.