Fix TestDnssd exit after last HandleResolved call #26138

arkq · 2023-04-18T09:01:46Z

Problem

After last service has been resolved the test case calls chip::DeviceLayer::PlatformMgr().Shutdown() without waiting for event loop termination. So, from time to time we've got race condition with abort:

[1681807067.000859][1049843:1049844] CHIP:DL: Avahi resolve found
Service[8] at [127.0.0.1]:80
[1681807067.002774][1049843:1049844] CHIP:DL: writing settings to file (/tmp/chip_counters.ini-vRqojS)
[1681807067.002981][1049843:1049844] CHIP:DL: renamed tmp file to file (/tmp/chip_counters.ini)
[1681807067.003022][1049843:1049844] CHIP:DL: NVS set: chip-counters/total-operational-hours = 0 (0x0)
[1681807067.003045][1049843:1049844] CHIP:SPT: VerifyOrDie failure at src/include/platform/internal/GenericPlatformManagerImpl_POSIX.ipp:312: mState.load(std::memory_order_relaxed) == State::kStopped
Aborted (core dumped)

It happens because the chip::DeviceLayer::PlatformMgr().StopEventLoopTask() is called on the Matter thread, so this call can not wait for "self-thread join".

Changes

add Setup and Teardown for test runner (share matter stack between test cases added in the future)
do not call Shutdown() and exit() on the Matter thread
use dedicated browse context instead of global variables

Testing

Tested on Linux with avahi:

[1681807644.570791][1065218:1065219] CHIP:DL: Avahi resolve found                                                                                                                                                    
Service[8] at [127.0.0.1]:80                                                                                                                                                                                         
[1681807644.571849][1065218:1065219] CHIP:DL: End EventLoop                                                                                                                                                          
'#3:','Test Dnssd::PubSub','PASSED'                                                                                                                                                                                  
'#6:','0','1'                                                                                                                                                                                                        
'#7:','0','59'                                                                                                                                                                                                       
[1681807644.573872][1065218:1065218] CHIP:DL: writing settings to file (/tmp/chip_counters.ini-733Ciz)                                                                                                               
[1681807644.574124][1065218:1065218] CHIP:DL: renamed tmp file to file (/tmp/chip_counters.ini)                                                                                                                      
[1681807644.574194][1065218:1065218] CHIP:DL: NVS set: chip-counters/total-operational-hours = 0 (0x0)                                                                                                               
[1681807644.574221][1065218:1065218] CHIP:DL: Inet Layer shutdown                                                                                                                                                    
[1681807644.574241][1065218:1065218] CHIP:DL: BLE shutdown                                                                                                                                                           
[1681807644.574314][1065218:1065218] CHIP:DL: System Layer shutdown
'#4:','Teardown          ','PASSED'
'#6:','0','1'
'#7:','0','61'

github-actions · 2023-04-21T20:00:54Z

PR #26138: Size comparison from 65f075c to b3fa580

Increases (1 build for cc32xx)

platform	target	config	section	`65f075c`	`b3fa580`	change	% change
cc32xx	lock	CC3235SF_LAUNCHXL	.debug_info	20330829	20330830	1	0.0

Full report (1 build for cc32xx)

platform	target	config	section	`65f075c`	`b3fa580`	change
cc32xx	lock	CC3235SF_LAUNCHXL		0	0	0
			(read only)	643249	643249	0
			(read/write)	203848	203848	0
			.ARM.attributes	44	44	0
			.ARM.exidx	8	8	0
			.bss	197248	197248	0
			.comment	194	194	0
			.data	1480	1480	0
			.debug_abbrev	933224	933224	0
			.debug_aranges	87792	87792	0
			.debug_frame	302140	302140	0
			.debug_info	20330829	20330830	1
			.debug_line	`2687904`	`2687904`	0
			.debug_loc	`2838960`	`2838960`	0
			.debug_ranges	288072	288072	0
			.debug_str	3042335	3042335	0
			.ramVecs	780	780	0
			.resetVecs	64	64	0
			.rodata	104401	104401	0
			.shstrtab	232	232	0
			.stab	204	204	0
			.stabstr	441	441	0
			.stack	2048	2048	0
			.strtab	377963	377963	0
			.symtab	256976	256976	0
			.text	536728	536728	0

github-actions · 2023-04-25T15:27:44Z

PR #26138: Size comparison from b206949 to 1385a7e

Increases (1 build for cc32xx)

platform	target	config	section	`b206949`	`1385a7e`	change	% change
cc32xx	lock	CC3235SF_LAUNCHXL	.debug_info	19489249	19489250	1	0.0

Full report (1 build for cc32xx)

platform	target	config	section	`b206949`	`1385a7e`	change
cc32xx	lock	CC3235SF_LAUNCHXL		0	0	0
			(read only)	601114	601114	0
			(read/write)	204132	204132	0
			.ARM.attributes	44	44	0
			.ARM.exidx	8	8	0
			.bss	197544	197544	0
			.comment	206	206	0
			.data	1468	1468	0
			.debug_abbrev	956756	956756	0
			.debug_aranges	103416	103416	0
			.debug_frame	349704	349704	0
			.debug_info	19489249	19489250	1
			.debug_line	2678035	2678035	0
			.debug_line_str	513	513	0
			.debug_loc	33340	33340	0
			.debug_loclists	1501882	1501882	0
			.debug_ranges	4984	4984	0
			.debug_rnglists	96008	96008	0
			.debug_str	3024877	3024877	0
			.ramVecs	780	780	0
			.resetVecs	64	64	0
			.rodata	104170	104170	0
			.shstrtab	265	265	0
			.stack	2048	2048	0
			.strtab	477531	477531	0
			.symtab	285936	285936	0
			.text	494824	494824	0

bzbarsky-apple

This is beautiful. Much better without the extra threading bits, and easier to scale!

src/platform/tests/TestConfigurationMgr.cpp

src/platform/tests/TestDnssd.cpp

arkq · 2023-04-26T08:17:04Z

This is beautiful. Much better without the extra threading bits, and easier to scale!

I'm glad that you like it. That was my goal, to make this suite scalable.

github-actions · 2023-04-26T08:38:36Z

PR #26138: Size comparison from b206949 to b637b4d

Increases (1 build for cc32xx)

platform	target	config	section	`b206949`	`b637b4d`	change	% change
cc32xx	lock	CC3235SF_LAUNCHXL	.debug_info	19489249	19489250	1	0.0

Full report (1 build for cc32xx)

platform	target	config	section	`b206949`	`b637b4d`	change
cc32xx	lock	CC3235SF_LAUNCHXL		0	0	0
			(read only)	601114	601114	0
			(read/write)	204132	204132	0
			.ARM.attributes	44	44	0
			.ARM.exidx	8	8	0
			.bss	197544	197544	0
			.comment	206	206	0
			.data	1468	1468	0
			.debug_abbrev	956756	956756	0
			.debug_aranges	103416	103416	0
			.debug_frame	349704	349704	0
			.debug_info	19489249	19489250	1
			.debug_line	2678035	2678035	0
			.debug_line_str	513	513	0
			.debug_loc	33340	33340	0
			.debug_loclists	1501882	1501882	0
			.debug_ranges	4984	4984	0
			.debug_rnglists	96008	96008	0
			.debug_str	3024877	3024877	0
			.ramVecs	780	780	0
			.resetVecs	64	64	0
			.rodata	104170	104170	0
			.shstrtab	265	265	0
			.stack	2048	2048	0
			.strtab	477531	477531	0
			.symtab	285936	285936	0
			.text	494824	494824	0

src/platform/tests/TestDnssd.cpp

github-actions · 2023-04-26T15:36:33Z

PR #26138: Size comparison from b206949 to c60cab3

Full report (1 build for mbed)

platform	target	config	section	`b206949`	`c60cab3`
mbed	lock-app	CY8CPROTO_062_4343W+release	(read only)	6224	6224
			(read/write)	2492536	2492536
			.bss	216272	216272
			.data	5144	5144
			.text	`1455220`	`1455220`

src/platform/tests/TestDnssd.cpp

github-actions · 2023-04-26T19:28:54Z

PR #26138: Size comparison from b1009b4 to f5dbb09

Decreases (1 build for cc32xx)

platform	target	config	section	`b1009b4`	`f5dbb09`	change	% change
cc32xx	lock	CC3235SF_LAUNCHXL	.debug_info	19489250	19489249	-1	-0.0

Full report (1 build for cc32xx)

platform	target	config	section	`b1009b4`	`f5dbb09`	change	% change
cc32xx	lock	CC3235SF_LAUNCHXL		0	0	0	0.0
			(read only)	601114	601114	0	0.0
			(read/write)	204132	204132	0	0.0
			.ARM.attributes	44	44	0	0.0
			.ARM.exidx	8	8	0	0.0
			.bss	197544	197544	0	0.0
			.comment	206	206	0	0.0
			.data	1468	1468	0	0.0
			.debug_abbrev	956756	956756	0	0.0
			.debug_aranges	103416	103416	0	0.0
			.debug_frame	349704	349704	0	0.0
			.debug_info	19489250	19489249	-1	-0.0
			.debug_line	2678035	2678035	0	0.0
			.debug_line_str	513	513	0	0.0
			.debug_loc	33340	33340	0	0.0
			.debug_loclists	1501882	1501882	0	0.0
			.debug_ranges	4984	4984	0	0.0
			.debug_rnglists	96008	96008	0	0.0
			.debug_str	3024877	3024877	0	0.0
			.ramVecs	780	780	0	0.0
			.resetVecs	64	64	0	0.0
			.rodata	104170	104170	0	0.0
			.shstrtab	265	265	0	0.0
			.stack	2048	2048	0	0.0
			.strtab	477531	477531	0	0.0
			.symtab	285936	285936	0	0.0
			.text	494824	494824	0	0.0

src/platform/tests/TestDnssd.cpp

github-actions · 2023-04-28T06:38:13Z

PR #26138: Size comparison from b1009b4 to 8d97998

Increases (1 build for mbed)

platform	target	config	section	`b1009b4`	`8d97998`	change	% change
mbed	lock-app	CY8CPROTO_062_4343W+release	(read/write)	2492536	`2494568`	2032	0.1
			.bss	216272	216296	24	0.0
			.text	`1455220`	1457252	2032	0.1

Full report (1 build for mbed)

platform	target	config	section	`b1009b4`	`8d97998`	change	% change
mbed	lock-app	CY8CPROTO_062_4343W+release	(read only)	6224	6224	0	0.0
			(read/write)	2492536	`2494568`	2032	0.1
			.bss	216272	216296	24	0.0
			.data	5144	5144	0	0.0
			.text	`1455220`	1457252	2032	0.1

github-actions bot added the platform label Apr 18, 2023

arkq force-pushed the mdns-fix-shutdown branch from 26aa41e to b3fa580 Compare April 21, 2023 19:47

arkq added 5 commits April 25, 2023 16:08

Fix typo in comment

7c98d3c

Use dedicated context instead of global state

5595a24

Use Setup/Teardown for DNS-SD test suite

8dafff9

Test timeout in case of Avahi not running

97ac5d9

Use ChipDnssdStopBrowse before exit

1385a7e

arkq force-pushed the mdns-fix-shutdown branch from b3fa580 to 1385a7e Compare April 25, 2023 15:06

arkq marked this pull request as ready for review April 25, 2023 19:09

bzbarsky-apple approved these changes Apr 25, 2023

View reviewed changes

src/platform/tests/TestConfigurationMgr.cpp Outdated Show resolved Hide resolved

src/platform/tests/TestDnssd.cpp Outdated Show resolved Hide resolved

arkq added 2 commits April 26, 2023 10:19

Fix typo in comment

51a3a51

Cancel timer before stopping event loop

b637b4d

arkq commented Apr 26, 2023

View reviewed changes

src/platform/tests/TestDnssd.cpp Outdated Show resolved Hide resolved

Avoid deadlock when stopping event loop on non-matter thread

c60cab3

bzbarsky-apple reviewed Apr 26, 2023

View reviewed changes

src/platform/tests/TestDnssd.cpp Show resolved Hide resolved

Update src/platform/tests/TestDnssd.cpp

f5dbb09

arkq force-pushed the mdns-fix-shutdown branch from 149863d to f5dbb09 Compare April 26, 2023 19:17

andy31415 reviewed Apr 27, 2023

View reviewed changes

src/platform/tests/TestDnssd.cpp Outdated Show resolved Hide resolved

andy31415 approved these changes Apr 27, 2023

View reviewed changes

pullapprove bot added review - approved and removed review - pending labels Apr 27, 2023

Remove unnecessary comment

8d97998

arkq merged commit 6ec5f81 into project-chip:master Apr 28, 2023

arkq deleted the mdns-fix-shutdown branch April 28, 2023 08:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix TestDnssd exit after last HandleResolved call #26138

Fix TestDnssd exit after last HandleResolved call #26138

arkq commented Apr 18, 2023 •

edited

Loading

github-actions bot commented Apr 21, 2023

github-actions bot commented Apr 25, 2023

bzbarsky-apple left a comment

arkq commented Apr 26, 2023

github-actions bot commented Apr 26, 2023

github-actions bot commented Apr 26, 2023

github-actions bot commented Apr 26, 2023

github-actions bot commented Apr 28, 2023

Fix TestDnssd exit after last HandleResolved call #26138

Fix TestDnssd exit after last HandleResolved call #26138

Conversation

arkq commented Apr 18, 2023 • edited Loading

Problem

Changes

Testing

github-actions bot commented Apr 21, 2023

github-actions bot commented Apr 25, 2023

bzbarsky-apple left a comment

Choose a reason for hiding this comment

arkq commented Apr 26, 2023

github-actions bot commented Apr 26, 2023

github-actions bot commented Apr 26, 2023

github-actions bot commented Apr 26, 2023

github-actions bot commented Apr 28, 2023

arkq commented Apr 18, 2023 •

edited

Loading