Unify the timeouts for the DAP tests

[JDevlieghere]

Various DAP tests are specifying their own timeouts, with values ranging from "1" to "20". Most of them seem arbitrary, but some come with a comment. Even when they're supposed to be intentional, I'm skeptical that they're beneficial. The performance characters of running these tests in CI are unpredictable (they generally run much slower than developers expect) and really not something we can make assumptions about. I suspect these timeouts are a contributing factor to the flakiness of the DAP tests.

Given the nature of DAP, using some kind of timeout is unavoidable. We should have a single (probably really big) timeout in the server, and abstract this away for test authors. This will make it harder to quickly catch timeouts during development, but that's something that can be addressed by either overwriting the timeout value for local development or setting the per-test lit timeout.

[llvmbot]

@llvm/issue-subscribers-lldb-dap

Author: Jonas Devlieghere (JDevlieghere)

Various DAP tests are specifying their own timeouts, with values from ranging from "1" to "20". Most of them seem arbitrary, but some come with a comment. Even when they're supposed to be intentional, I'm skeptical that they're beneficial. The performance characters of running these tests in CI are unpredictable (they generally run much slower than developers expect) and really not something we can make assumptions about. I suspect these timeouts are a contributing factor to the flakiness of the DAP tests.

Given the nature of DAP, using some kind of timeout is unavoidable. We should have a single (probably really big) timeout in the server, and abstract this away for test authors. This will make it harder to quickly catch timeouts during development, but that's something that can be addressed by either overwriting the timeout value for local development or setting the per-test lit timeout.

[adrian-prantl]

Nit: I've seen values going as low as 0.25s, which is definitely stretching it for any already stressed CI system.

[walter-erquinigo]

what about using the same timeout that exists for gdb-remote packets?

[ashgti]

We have a default timeout value:

llvm-project/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py

Line 21 in e160b2a

DEFAULT_TIMEOUT = 10 * (10 if ("ASAN_OPTIONS" in os.environ) else 1)

and most places that take a timeout default to that.

There are some places where we have a sleep wait as well, any place that calls 'wait_until':

llvm-project/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py

Line 133 in e160b2a

def wait_until(

DebugCommunication does have a number of methods with timeouts, but the default is in the test case not the DebugCommunication class itself currently.

If we wanted to try to improve this, I wonder if we should look at using the async python. async def was added in Python 3.8, and lets us treat some of this as truly async code. Right now we have a lot of synchronization between the reader thread and the main test thread. With the async code, that doesn't require multiple threads, since the code becomes event driven instead. Just a thought.

[JDevlieghere]

Using async sounds interesting. My gut feeling is that the DAP tests are unreliable because it's easy to forget synchronization because Python is forgiving and on a reasonably fast machine (and locally) we simply don't notice. If that's true, then the only way to eliminate this is by either making that pain felt by us as developers or designing it away. Obviously the latter is preferable, so anything that gets us closer to that is a win.

[ashgti]

Do we have any stats on which tests are the most unreliable in CI? Or which platforms have the most issues?

I suppose one metric for this would be checking which tests are disabled on which platforms from the decorators in the tests.

[boomanaiden154]

Do we have any stats on which tests are the most unreliable in CI? Or which platforms have the most issues?

We should have good data for this from premerge soon (ie next week or two). Whether or not that is representative is a different question, but either way it’s an important use case for the community.