Skip to content
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

Worker tests: use unique pointers when possible #1421

Merged
merged 18 commits into from
Jul 6, 2024
Merged

Conversation

ibc
Copy link
Member

@ibc ibc commented Jul 1, 2024

(WIP)

Details

  • Avoid having allocated pointers outside unique_ptr pointers.
  • So caution with all those mediasoup Parse() static methods that return an allocated pointer.
  • TestRtpStreamSend.cpp: Use unique_ptr instead of shared_ptr.

(WIP)

### Details

- Avoid having allocated pointers outside `unique_ptr` pointers.
- So caution with all those mediasoup `Parse()` static methods that return an allocated pointer.
- Working on `TestXr.cpp`.
- `TestRtpStreamSend.cpp`: Use `unique_ptr` instead of `shared_ptr`.
@ibc
Copy link
Member Author

ibc commented Jul 1, 2024

Questions

  • Why does TestRtpStreamSend.cpp use shared_ptr instead of unique_ptr?
  • Why does NOT TestRtpStreamSend.cpp use xxx_ptr in cases like this one?
    auto* packet = RtpPacket::Parse(rtpBuffer1, 1500);
  • How is possible that ASAN doesn't report any problem in TestRtpStreamSend.cpp despite such a packet is not being deleted?

@ibc ibc requested a review from jmillan July 1, 2024 18:55
@jmillan
Copy link
Member

jmillan commented Jul 2, 2024

Why does TestRtpStreamSend.cpp use shared_ptr instead of unique_ptr?

Because a shared_ptr is what the API expects. Check the method being called.

Why does NOT TestRtpStreamSend.cpp use xxx_ptr in cases like this one?

That pointer is owned by the caller of CreateRtpPacket(), which is handled by a unique_ptr

@jmillan jmillan marked this pull request as ready for review July 2, 2024 08:17
@ibc
Copy link
Member Author

ibc commented Jul 2, 2024

Why does TestRtpStreamSend.cpp use shared_ptr instead of unique_ptr?
Because a shared_ptr is what the API expects. Check the method being called.

I don't understand. I literally mean that I replaced some shared_ptr with unique_ptr in that files and it compiles, tests pass and no ASAN warnings (in that test file).

@ibc ibc marked this pull request as draft July 2, 2024 09:30
@ibc
Copy link
Member Author

ibc commented Jul 3, 2024

@jmillan, ongoing changes in TestXr.cpp in this PR are producing a SIGSEV when running worker tests (no need to run them with ASAN stuff):

-------------------------------------------------------------------------------
Scenario: RTCP XrDelaySinceLastRt parsing
  create RRT
-------------------------------------------------------------------------------
../../../test/src/RTC/RTCP/TestXr.cpp:131
...............................................................................

../../../test/src/RTC/RTCP/TestXr.cpp:131: FAILED:
  {Unknown expression after the reported line}
due to a fatal error condition:
  SIGSEGV - Segmentation violation signal

===============================================================================
test cases:     39 |     38 passed | 1 failed
assertions: 592664 | 592663 passed | 1 failed

My guess is that we are creating a XR packet and we are passing reports to it created with unique_ptr, however the XR packet will delete those pointers by itself when it's deleted. Does it make sense?

@ibc
Copy link
Member Author

ibc commented Jul 3, 2024

My guess is that we are creating a XR packet and we are passing reports to it created with unique_ptr, however the XR packet will delete those pointers by itself when it's deleted. Does it make sense?

Confirmed, and fixed here 281054c

@jmillan
Copy link
Member

jmillan commented Jul 4, 2024

@ibc, this PR is ready to merge, plus there are 0 issues when running make test-asan-address

@ibc
Copy link
Member Author

ibc commented Jul 4, 2024

My aim in this PR is to use unique/share_ptr in all test files, not just in TestXr.cpp.

@ibc
Copy link
Member Author

ibc commented Jul 6, 2024

@jmillan thanks a lot for your commits. Just one thing, we don't need to include <memory>, it's already done in common.hpp.

@ibc ibc mentioned this pull request Jul 6, 2024
@ibc
Copy link
Member Author

ibc commented Jul 6, 2024

This is done. I've enabled test-asan-address in CI (only for Linux) and will merge if CI passes.

@ibc ibc marked this pull request as ready for review July 6, 2024 11:59
@ibc ibc merged commit 8b46a2b into v3 Jul 6, 2024
41 checks passed
@ibc ibc deleted the use-unique-ptr-in-wirker-tests branch July 6, 2024 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants