Thread safe singleton

[muhammetasan]

• singleton is more efficient and thread safe this way.
• instance of logwriter can be initialized to reduce its scope.

[francescmm]

Hi, I've change this PR to a develop branch since I'd need to modify the Readme.md. The changes are okay and I'll merge them today :+1:

[francescmm]

@muhammetasan With this change the logger crashes when an app closes. Please take a look into it. I'll revert the changes so you can fix it.

[muhammetasan]

Actually I have a workaround for this. making static instance as static pointer would help or calling thread stop in destructor may help. But in existing implementation there is a memory leak. Objects are not deleted and thats why you do not get "Qthread is destructed while you are running" error. But here there are more significant design problems. For instance, mutex is shared publicly . This is dangerous. Secondly you are inheriting from Qthread which is not the correct way to make use of Qthread. I will change the design little bit. Also if you log thread ids, you can see that all logs are written to file by same thread, namely GUI thread. I think this is not something you wanted to do.

[francescmm]

@muhammetasan I have several nuances to add to your comment.

Actually I have a workaround for this. making static instance as static pointer would help or calling thread stop in destructor may help. But in existing implementation there is a memory leak. Objects are not deleted and thats why you do not get "Qthread is destructed while you are running" error.

I'm not sure what do you mean with "making static instance as static pointer". How it is now is a static instance, may be I'm missing something.

By the other hand, QLogger must be stop using the closeLogger method. The only thing that it's true that could be leaking, is that I'm not clearing the logger writers. I can fix that. However that's completely unrelated with "Qthread is destructed while you are running". That happens when you destroy the QLogger and is still used and that depends on where you close the logger.

But here there are more significant design problems. For instance, mutex is shared publicly . This is dangerous.

It's dangerous that someone access it without any good reason or without knowing what they are doing. But I wouldn't consider it design problem. Implementation problem perhaps. User problem for sure! :smile:

The solution is to create a class that actually calls the log manager and the log writer. Put the mutex private and make the new class friend of QLoggerManager. That's a huge change in the design that I'm not considering know.

I could also split the manager in the "manager" and the "writer" what would be more accurate and solve that problem. Then make the manager public and the writer private as a helper class in the cpp file. There are some solutions to the mutex, of curse.

But, if someone just acts on the mutex freely and that causes problems is not because a bad design, it's because they are not respecting how the logger works. I don't want to go here and there forbidding things.

Said that, I will take some time in the future to change it a bit but for now that remains.

Secondly you are inheriting from Qthread which is not the correct way to make use of Qthread. I will change the design little bit.

Actually, don't do it. Don't change the design because it's actually designed like that. It is correct and even there were some sort of crusade to avoid inheriting from QThread is completely fine. Here you can find an example of valid usage. And even Qt does it in their examples

Also if you log thread ids, you can see that all logs are written to file by same thread, namely GUI thread. I think this is not something you wanted to do.

From the very beginning when you log from the UI thread, that will attach that thread to the UI. I'm not saying that QLogger is multi-threading. It is not and it is not my intention at all, since I'd like to log from the UI. QLogger is thread-safe, that is a different thing.

[muhammetasan]

It's dangerous that someone access it without any good reason or without knowing what they are doing. But I wouldn't consider it design problem. Implementation problem perhaps. User problem for sure! smile

You can not make this assumption. Your QLogger is a utility library which you are publishing. And not taking care of the public mutex.

In Qthread example you shared they are extending Qthread. What are you doing with QTthread ? What is your purpose on inheriting QThread ? Are you implementing run() method? No. They are running concurrent operation in run method. But what you are doing is : You inherit LoggerWriter from QThread. You call start() in constructor. Then run() function which is empty in your case is called and finishes immediately. In the rest of the lifecycle of loggerwriter object it acts as a regular object. no thread functionality is used. You are not using any thread functionality , why you are inheriting from QThread ?

[muhammetasan]

For static pointer instance I mean : static QLoggerWriter * instance = new QLoggerWriter(); return instance; this time the destructor is not called when program exits. This is memory leak, same as your case. But after process finished the memory is freed by operating system. But your objects are not destroyed. Thats why you do not get Qthread running error.

[francescmm]

I think we're a bit out of scope, so I'll try to reply all the comments at once and feel free to address all the issues you see in a new PR and we can continue the conversation there.

You can not make this assumption. Your QLogger is a utility library which you are publishing. And not taking care of the public mutex.

I can because it's my library. Everybody is free to use it and ask for help. If someone doesn't fully understand the code, better not to touch it. There is no more reasoning.

In Qthread example you shared they are extending Qthread.

I gave you two examples to tell you that your comment (Secondly you are inheriting from Qthread which is not the correct way to make use of Qthread. I will change the design little bit.) is false, wrong. Don't extend that to a full justification. Actually, you should stop making general statements since they are not always right or wrong. Every case is different.

What are you doing with QTthread ? What is your purpose on inheriting QThread ? Are you implementing run() method? No. They are running concurrent operation in run method. But what you are doing is : You inherit LoggerWriter from QThread. You call start() in constructor. Then run() function which is empty in your case is called and finishes immediately. In the rest of the lifecycle of loggerwriter object it acts as a regular object. no thread functionality is used. You are not using any thread functionality , why you are inheriting from QThread ?

That's true: nowadays I'm not using QThread functionality and is acting as a regular object. Just a correction: is QLoggerManager who inherits from QThread, not QLoggerWriter. I wll take a look to it, or feel free to create a PR to remove it.

For static pointer instance I mean : static QLoggerWriter * instance = new QLoggerWriter(); return instance; this time the destructor is not called when program exits. This is memory leak, same as your case. But after process finished the memory is freed by operating system. But your objects are not destroyed. Thats why you do not get Qthread running error.

This goes to C++ then. If you do new you need to delete to free the memory. I will repeat it in case you miss it:

QLogger must be stop using the closeLogger method. The only thing that is leaking is the only thing you didn't pointed out: I'm not clearing the logger writers. That, I will fix it.