roc-streaming / roc-toolkit

Real-time audio streaming over the network.
https://roc-streaming.org
Mozilla Public License 2.0
1.06k stars 211 forks source link

Assign names to threads #618

Open gavv opened 11 months ago

gavv commented 11 months ago

pthread allows to assign names to threads, which is very handy for debugging. For example, gdb will display this names.

We always create threads using core::Thread class. We can do the following:

This SO answer gives an idea how to do it. Similar to Thread::get_tid, we should use ifdefs for different platforms.

8julie commented 11 months ago

Hey! I might need a couple days to try and figure things out, but are you guys still looking for help regarding this issue?

gavv commented 11 months ago

Yes. You're welcome, thanks!

8julie commented 11 months ago

Quick question: Do you have any ideas on how long the thread name length should be?

gavv commented 11 months ago

Since we control thread names, we can set some short limit.

echo -n roc_network_loop | wc -c
16

I think limit of 32 characters would be fine.

8julie commented 11 months ago

Hi! I have a quick question about adding const char *name to the Thread constructor. If I add this, the classes that inherit from the base class Thread would all need to be rewritten with name passed through them, which I don't think would be very convenient.
Edit: I also tried seeing if default arguments would be possible and I don't believe it is possible for const values

Was the purpose of adding this new parameter to prevent Threads from being renamed? Do you want Thread to be rename-able? I think there's a way for either way to work, but I'm unsure what is desired here. Let me know! Also this is my first time contributing to open source so I'm not too sure how things are supposed to go, so do let me know if there is anything else I could do.

gavv commented 11 months ago

If I add this, the classes that inherit from the base class Thread would all need to be rewritten with name passed through them, which I don't think would be very convenient.

Actually this is the intention of the task - force all roc threads to have names, so that you can see them in debugger.

Was the purpose of adding this new parameter to prevent Threads from being renamed? Do you want Thread to be rename-able?

I don't see any reason to allow renaming. This names are just static strings for convenience for logging/debugging/inspecting stacktraces.

Also this is my first time contributing to open source so I'm not too sure how things are supposed to go, so do let me know if there is anything else I could do.

No worries, you're welcome :)

8julie commented 11 months ago

I see, do you want me to fix all of those threads as well so that there is a name for the constructors?

gavv commented 10 months ago

I see, do you want me to fix all of those threads as well so that there is a name for the constructors?

Yep, this is the motivation of this issue. I think there are just about 4 threads actually + more in tests, but their names are not that important.

8julie commented 10 months ago

Thanks for the clarification! Also, I edited control_task_queue among some other threads and started building + testing but I'm not too familiar with how debugging works (I've never used scons before). I couldn't find much in the website, could you let me know how I could perhaps set up debugging?

gavv commented 10 months ago

See here https://roc-streaming.org/toolkit/docs/building/developer_cookbook.html

Pass --enable-debug to build debug version of code

And you can find test binaries at: ./bin/x86_64-pc-linux-gnu/roc-test-<name> (see page above for supported flags, e.g. to enable logs or select test)

So you can run those binaries under debugger as usual, e.g. gdb ./bin/x86_64-pc-linux-gnu/roc-test-core (or from IDE)

gavv commented 7 months ago

@8julie Hi! Do you still have plans on the issue?

8julie commented 7 months ago

Hello, Sorry I don't intend to do so anymore. Please remove me from this thread, thank you for helping me in the time was involved

On Thu, Feb 8, 2024 at 2:28 AM Victor Gaydov @.***> wrote:

@8julie https://github.com/8julie Hi! Do you still have plans on the issue?

— Reply to this email directly, view it on GitHub https://github.com/roc-streaming/roc-toolkit/issues/618#issuecomment-1933778212, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJIRPFOLHWFPFYQFVSLQ2W3YSSSERAVCNFSM6AAAAAA6DXJRVOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZTG43TQMRRGI . You are receiving this because you were mentioned.Message ID: @.***>

-- Minh Anh Bui

dmklepp commented 6 months ago

Hello, I'm new to this project. I will be attempting to fix this issue. Thank you!

gavv commented 5 months ago

@dmklepp Great, thanks!

gavv commented 3 months ago

@dmklepp Hi, still have plans on this?

gavv commented 2 months ago

Unassigning, so that someone could pick this up.

Here is the link to abandoned PR: #716