Page MenuHomePhabricator

[LLDB] Fix sync issue in TestVSCode_launch.test_progress_events
ClosedPublic

Authored by omjavaid on Mar 29 2021, 4:49 AM.

Details

Summary

This fixes flakiness in TestVSCode_launch.test_progress_events
vscode.progress_events some times failed to populate in time for the for the follow up iteration.

Adding a minor delay before the the for the loop fixes the issue.

Diff Detail

Event Timeline

omjavaid requested review of this revision.Mar 29 2021, 4:49 AM
omjavaid created this revision.
omjavaid added a subscriber: lldb-commits.
This revision is now accepted and ready to land.Mar 29 2021, 10:56 AM

Isn't there a better way to ensure synchronization here? Maybe, executing some command (setting a breakpoint, for instance), that will ensure that all symbols have been parsed?

Isn't there a better way to ensure synchronization here? Maybe, executing some command (setting a breakpoint, for instance), that will ensure that all symbols have been parsed?

That is what I attempted do in the test: set a breakpoint at 'main' which should trigger all of the needed events. Then we run to a breakpoint and hit it. and after we hit this breakpoint, then we check the progress events.

One thing we could do to figure things out would be to dump the vscode.txt file for this test into the failing test. This would allow us to see what the packets looked like when a test fails. Is there a cleanup function or something we can do in the VS code tests that allows a test case to know it has an error and add some stuff to the test suite output??

Isn't there a better way to ensure synchronization here? Maybe, executing some command (setting a breakpoint, for instance), that will ensure that all symbols have been parsed?

That is what I attempted do in the test: set a breakpoint at 'main' which should trigger all of the needed events. Then we run to a breakpoint and hit it. and after we hit this breakpoint, then we check the progress events.

Ok, I see what you mean. I looked (briefly) at the code, and my impression is that this dance will ensure that the progress events get /sent/, but they will not ensure that they are actually receieved/processed (as the receiving end is completely asynchronous).

The next question then becomes: is that intentional? If it is, then a delay is indeedall we can do (and I suspect we might need even more than 1s to sufficiently safe). Having progress events come after the operation that uses their results completes would be a bit weird, though it probably won't cause much problems in practice.

This situation actually reminds me of what we do with inferior stdout. That also used to be fully asynchronous (on several levels), but that ended up causing problems (for tests, at least), as we would try to assert that the inferior has written something after it hits a breakpoint, but that text would still be in flight. We solved that by explicitly flushing any pending output before sending the "stopped" event. One could imagine doing something similar in vscode as well -- before replying to a command (in this case, breakpoint set), check the progress listener, and flush any pending events.

One thing we could do to figure things out would be to dump the vscode.txt file for this test into the failing test. This would allow us to see what the packets looked like when a test fails. Is there a cleanup function or something we can do in the VS code tests that allows a test case to know it has an error and add some stuff to the test suite output??

There are several ways to do that. The simplest (and most useful, I think) would be to just stream the messages (both sent and received) to stdout if self.traceOn() is true. I don't think it will help much in this case, but it could be pretty useful in the future.

I am going ahead and commit this change till we find a better way to fix the sync issue. sleep time set to 2s now.

Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2021, 2:18 AM
labath added a comment.Apr 8 2021, 7:34 AM

The test is still flaky (http://lab.llvm.org:8011/#/builders/68/builds/10124, http://lab.llvm.org:8011/#/builders/68/builds/10118, http://lab.llvm.org:8011/#/builders/68/builds/10113, ...) In fact, I think it has gotten flakier since the timeouts were added... I'm disabling it.