-
Notifications
You must be signed in to change notification settings - Fork 137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed microservice cleanup and tests #660
Conversation
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will have started by now, so should be properly destroyed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@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. |
@kozlovic Thx for suggesting natsLib as the placeholder for the globals. I like it and will definitely keep it. I think I fixed (or explained) all the other PR feedback, thank you! So silly that I forgot about that 2 questions:
|
If you run with 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.
I use the same, but you may want to check Settings and look for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some little changes and I think we are good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes race issues on cleanup, and a few small bugs.
NOTE:
Test_MicroServiceStopsOnClosedConn
andTest_MicroServiceStopsWhenServerStops
still appear to take too long, but I am not sure what's causing the wait/timeout there.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 againstdev
, dev...lev-tmp2 since this PR mostly rolls back prior changes.micro_ErrorFromStatus
_
prefix)