vsronin/chatexample

chatexample's Issues

Race condition in shutdown sequence

In the commonlib branch whenever the threads in the pool start to exit there's a race condition that has to be fixed. The problem is that QObject::destroyed() is emitted from the thread's context but after the QThread::finished, meaning the code in ThreadPool::~ThreadPool will be executed before the QObject::destroyed is processed, leading to access to invalid memory.

Fix by removing the semaphore and using wait() on the threads in the pool.

SGaist review

Purely from personal taste:

• Missing white spaces in headers and code files
• Methods, variables classifications in class declaration not coherent
• Public/Protected/Private ordering looks strange to me

Overall:

• MessageTypes, why a vector and a map ?
• What about having links to the documentation in the code comments ?
• What about using a helper function to enable/disable widgets in the interface ?

Make the server a Widget application

It currently is a console app but there's a common misunderstanding around of the server blocking if not threaded, using a GUI should illustrate this is not true

JKSH Review

What do you guys think of the Describe ReadyRead paragraph?

Content is good. +1 for pointing out that the buffer might have contain than less/more than 1 message, and for providing a solution.

Some minor polish can be applied to the language, e.g. "This is what we are using here to detect if what we read with socketStream >> jsonData; was actually a full JSON or just a partial." sounds funny. Perhaps "We use this to check if socketStream >> jsonData; retrieved a full JSON document or not."

+1 for teaching me about QDataStream transactions!

It's just my coolness leaking through the code ;P

We need 2 separate examples: One for n00bs and one for l33t h4x0rz! (as in actual l33ts, not wannabes)

see if moving stuff into a shared lib made it simpler/more concise/more straightforward ...

Using a shared lib made the client and server more high level and easier to read, and made it easier to make updates. This came at the cost of a few extra requirements:

The reader must be comfortable with editing+building libraries and be happy with the Q_DECL_IMPORT/Q_DECL_EXPORT macros
The reader must understand QSharedData first
The reader should ideally be capable of designing their own class inheritance tree (or at least be able to read/follow an implementation)

you do know it's much easier to read with the overloads though :|

Hmm... you're right.

After thinking about it a bit more, I think the the example would work best in 2 stages:

Stage 1: Teach the reader how to use QTcpSocket/QTcpServer and threads.

• Bare-bones functionality
• No custom message classes or QSharedData (just use raw QJsonObjects and assume that all data is valid)
• No separate library

Stage 2: Teach the reader how to architect a library + application

• Everything currently in the commonlib branch
• Reinstate the overloads for maximum flexibility

The recent improvements in commonlib are very suitable for Stage 2.