Fixed microservice cleanup and tests #660

levb · 2023-06-03T14:12:16Z

Fixes race issues on cleanup, and a few small bugs.

NOTE:

Test_MicroServiceStopsOnClosedConn and Test_MicroServiceStopsWhenServerStops still appear to take too long, but I am not sure what's causing the wait/timeout there.
I've seen Test_MicroAsyncErrorHandler hang in CI once, was not able to reproduce nor debug the problem.

For non-micro changes (nats.[ch], opts.[ch], it may be easier to view against dev, dev...lev-tmp2 since this PR mostly rolls back prior changes.

Added a global list of running services, use it for all connection callbacks.
Reworked service/endpoint locking and refcounting.
Improved synchronization in tests
Fixed micro_ErrorFromStatus
Renamed some static methods for consistency (_ prefix)

correct fix? wip minimized list.txt minimized list.txt minimized list.txt wip wip more wip more+ wip more+oops wip more+oops+printf

levb · 2023-06-04T12:59:29Z

src/micro.c

    MICRO_CALL(err, micro_init_monitoring(m));
    MICRO_CALL(err, microService_AddEndpoint(m, cfg->Endpoint));

    if (err != NULL)
    {
-        micro_release_service(m);
+        microError_Ignore(microService_Destroy(m));


It will have started by now, so should be properly destroyed.

src/micro.c

…to lev-tmp2

kozlovic

I would really like for the global variables to be part of the library object so that they can be freed when the library is closed. Otherwise, you will have these "leaks" reported (still reachable really):

==1531== HEAP SUMMARY:
==1531==     in use at exit: 128 bytes in 3 blocks
==1531==   total heap usage: 435 allocs, 432 frees, 167,994 bytes allocated
==1531== 
==1531== 24 bytes in 1 blocks are still reachable in loss record 1 of 3
==1531==    at 0x4C33B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1531==    by 0x1CA2A6: natsHash_Create (in /home/ivan/cnats/build_linux/test/testsuite)
==1531==    by 0x1E9AEA: micro_AddService (in /home/ivan/cnats/build_linux/test/testsuite)
==1531==    by 0x191236: test_MicroServiceStopsOnClosedConn (in /home/ivan/cnats/build_linux/test/testsuite)
==1531==    by 0x1127E7: main (in /home/ivan/cnats/build_linux/test/testsuite)
==1531== 
==1531== 40 bytes in 1 blocks are still reachable in loss record 2 of 3
==1531==    at 0x4C33B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1531==    by 0x20DF1D: natsMutex_Create (in /home/ivan/cnats/build_linux/test/testsuite)
==1531==    by 0x1E9B0A: micro_AddService (in /home/ivan/cnats/build_linux/test/testsuite)
==1531==    by 0x191236: test_MicroServiceStopsOnClosedConn (in /home/ivan/cnats/build_linux/test/testsuite)
==1531==    by 0x1127E7: main (in /home/ivan/cnats/build_linux/test/testsuite)
==1531== 
==1531== 64 bytes in 1 blocks are still reachable in loss record 3 of 3
==1531==    at 0x4C33B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1531==    by 0x1CA2C8: natsHash_Create (in /home/ivan/cnats/build_linux/test/testsuite)
==1531==    by 0x1E9AEA: micro_AddService (in /home/ivan/cnats/build_linux/test/testsuite)
==1531==    by 0x191236: test_MicroServiceStopsOnClosedConn (in /home/ivan/cnats/build_linux/test/testsuite)
==1531==    by 0x1127E7: main (in /home/ivan/cnats/build_linux/test/testsuite)
==1531== 
==1531== LEAK SUMMARY:
==1531==    definitely lost: 0 bytes in 0 blocks
==1531==    indirectly lost: 0 bytes in 0 blocks
==1531==      possibly lost: 0 bytes in 0 blocks
==1531==    still reachable: 128 bytes in 3 blocks
==1531==         suppressed: 0 bytes in 0 blocks

src/micro.c

src/micro_error.c

src/nats.h

test/test.c

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>

kozlovic · 2023-06-05T21:31:16Z

@levb I have pushed a commit that is a step toward moving the global variables inside natsLib for proper free on nats_Close(). I did not look deep to see if we do need to retain/release the library, which we probably do if "micro" code is not retaining/releasing the connection (which itself does retain/release the library). The commit also includes the changes to the tests that I have referenced in the code review. If you decide not to take the changes related to the lib itself, at least you could keep the changes to the tests.

levb · 2023-06-06T14:10:38Z

@kozlovic Thx for suggesting natsLib as the placeholder for the globals. I like it and will definitely keep it. nc is already retained/released by _new/_free_service. I actually considered adding these (hash, etc) to connections but that seemed too intrusive/presumptuous even if services bind to single connections ATM. LMK what else (if anything) you think needs to be done for the proper cleanup there?

I think I fixed (or explained) all the other PR feedback, thank you! So silly that I forgot about that nats_Sleep and _onClosed and was sweating why it was taking too long!

2 questions:

What environment do you use to get detailed valgrind info? It doesn't seem visible in CI, and I'd love to fix the CI to display it.
What code formatter do you use? (re: whitespace misses). I use VSCode, and the default C formatter is not 100% compatible with the existing code, so I didn't use it thoroughly on my changes to shared files. I am 0/5 on the choice, can just use the same as you do, if possible.

kozlovic · 2023-06-06T17:01:10Z

What environment do you use to get detailed valgrind info? It doesn't seem visible in CI, and I'd love to fix the CI to display it.

If you run with ctest -T memcheck, it will use the default valgrind options that I define in the top-root CMakeLists.txt file: "--leak-check=full --track-fds=yes --show-reachable=yes --num-callers=50" (that you can see if you do a make edit_cache). There is currently no matrix in Travis to run those (instead I run the sanitize thread/address). I usually run a manual ctest -T memcheck on my Linux VM.

Keep in mind that if you have a real leak, then the current runs on Travis would find it (with sanitize address). What is not reported is the still reachable, for instance the global variables you added at one point.

What code formatter do you use? (re: whitespace misses). I use VSCode, and the default C formatter is not 100% compatible with the existing code, so I didn't use it thoroughly on my changes to shared files. I am 0/5 on the choice, can just use the same as you do, if possible.

I use the same, but you may want to check Settings and look for trim in the search, you should have "Text Editor/Files" and then you should check the box "Files: Trim Trailing Whitespaces" which says "When enabled, will trim trailing whitespace when saving a file".

kozlovic

I would recommend making the change I recommend about error's message. Note that if you do so, you may need to cast ("(char*) err->message") at line 68.

kozlovic

Just some little changes and I think we are good to go.

src/micro_error.c

kozlovic

LGTM!

levb added 20 commits May 26, 2023 15:57

try-1

c3677d2

correct fix? wip minimized list.txt minimized list.txt minimized list.txt wip wip more wip more+ wip more+oops wip more+oops+printf

wip

b3ee831

wip

7d732fb

wip2

de0f8a8

wip3

e531f28

wip4

0e85175

wip5

e4b80fd

reworked wip

71bb342

reworked wip2

c38ed27

reworked wip3

3d0d3ab

reworked wip4

cb68bad

reworked wip5

a4d89bc

reworked wip5

8f87ea2

reworked wip6

3ca86b6

Merge branch 'main' into lev-tmp2

5dab55d

reworked wip7

52c4881

reworked wip8

2019f51

reworked wip9

f76a3c5

Merge branch 'lev-microservice-proto' into lev-tmp2

863af3e

reworked wip10

104a46c

levb marked this pull request as draft June 3, 2023 14:12

levb added 7 commits June 3, 2023 07:24

reworked wip11

b80dc96

reworked wip12

5ae5e6f

reworked wip13

e2d41f0

reworked wip14

f1f1191

reworked wip15

8d66c11

wip nits

9da1b56

wip more nits

10fe2c5

levb commented Jun 4, 2023

View reviewed changes

levb changed the title ~~1045Lev tmp2~~ Fixed microservice cleanup and tests Jun 4, 2023

levb marked this pull request as ready for review June 4, 2023 13:24

levb requested a review from kozlovic June 4, 2023 13:24

Merge branch 'lev-microservice-proto' of github.com:nats-io/nats.c in…

b01f694

…to lev-tmp2

kozlovic requested changes Jun 5, 2023

View reviewed changes

Moving global variables inside natsLib for proper cleanup on close

f255707

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>

levb added 3 commits June 6, 2023 05:49

PR feedback sans global elimination

4e0709d

Merge branch 'lev-tmp2' of github.com:nats-io/nats.c into lev-tmp2

2474610

Restored num_services=5 in tests

394e4b2

levb requested a review from kozlovic June 6, 2023 14:04

kozlovic requested changes Jun 6, 2023

View reviewed changes

PR feedback: microError.message

fd95b7b

kozlovic requested changes Jun 7, 2023

View reviewed changes

src/micro_error.c Outdated Show resolved Hide resolved

levb added 2 commits June 7, 2023 14:13

PR feedback: nats_vsnprintf

e92657b

PR feedback: nats_vsnprintf, #2

338951d

levb requested a review from kozlovic June 7, 2023 22:00

kozlovic approved these changes Jun 9, 2023

View reviewed changes

levb merged commit a268db3 into lev-microservice-proto Jun 10, 2023

levb deleted the lev-tmp2 branch June 10, 2023 11:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed microservice cleanup and tests #660

Fixed microservice cleanup and tests #660

levb commented Jun 3, 2023 •

edited

Loading

levb Jun 4, 2023

kozlovic left a comment

kozlovic commented Jun 5, 2023

levb commented Jun 6, 2023

kozlovic commented Jun 6, 2023

kozlovic left a comment

kozlovic left a comment

kozlovic left a comment

Fixed microservice cleanup and tests #660

Fixed microservice cleanup and tests #660

Conversation

levb commented Jun 3, 2023 • edited Loading

levb Jun 4, 2023

Choose a reason for hiding this comment

kozlovic left a comment

Choose a reason for hiding this comment

kozlovic commented Jun 5, 2023

levb commented Jun 6, 2023

kozlovic commented Jun 6, 2023

kozlovic left a comment

Choose a reason for hiding this comment

kozlovic left a comment

Choose a reason for hiding this comment

kozlovic left a comment

Choose a reason for hiding this comment

levb commented Jun 3, 2023 •

edited

Loading