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

Multithreading race. It is not a solution. #3

Closed
heretic13 opened this issue May 7, 2015 · 18 comments
Closed

Multithreading race. It is not a solution. #3

heretic13 opened this issue May 7, 2015 · 18 comments
Assignees
Labels

Comments

@heretic13
Copy link

Run 2 or more processes.

They pseudo-paralell execute code

"socket->waitForConnected(1000)"

then they all go to the branch else
{
// If the connection is insuccessful, this is the main process
// So we create a Local Server
server = new QLocalServer();
server->removeServer(serverName);
server->listen(serverName);
QObject::connect(server, SIGNAL(newConnection()), this, SLOT(slotConnectionEstablished()));
}

and now we have 2 or more running processes;

@itay-grudev
Copy link
Owner

That is not possible, since the filesystem won't allow two processes to bind to the same local socket / file.

@itay-grudev
Copy link
Owner

Besides, if there are no running connections, the first process will bind to the file almost instantaneous and the second will manage to connect. The timeout was there since your system might have been under heavy load.

@heretic13
Copy link
Author

maybe.

but where is a error checking for that?
where checking return value?

2015-05-07 17:07 GMT+03:00 Itay Grudev notifications@github.com:

That is not possible, since the filesystem won't allow two processes to
bind to the same local socket / file.


Reply to this email directly or view it on GitHub
#3 (comment)
.

@heretic13
Copy link
Author

auto server1 = new QLocalServer();
server1->removeServer("111");
server1->listen("111");

auto server2 = new QLocalServer();
server2->removeServer("111");
server2->listen("111");

......

not blocking. I may reproduce this code many hundred copies.

@itay-grudev
Copy link
Owner

Yes, because in your code each subsequent call removes the previous server. Now add the check for connection.

QLocalServer *server1 = new QLocalServer();
server1->removeServer("111");
server1->listen("111");

QLocalServer *server2;
QLocalSocket *socket = new QLocalSocket();

socket->connectToServer("111");
if(socket->waitForConnected(1000)){
    server2 = new QLocalServer();
    server2->removeServer("111");
    server2->listen("111");
}

With the check it won't work. The filesystem provides a lock feature which is a mutex by itself. Therefore no race would occur since the file is locked and accepting connections.

The only reason a connection is not established within 1 second is that the "old server" process has crashed. In which case a new instance would be necessary.

@heretic13
Copy link
Author

run 2 threads (processes) in one code:

// Initial state = no running servers

QLocalSocket *socket1 = new QLocalSocket(); // thread 1
QLocalSocket *socket2 = new QLocalSocket(); // thread 2

socket1->connectToServer("111"); // thread 1
socket2->connectToServer("111"); // thread 2

if(!socket1->waitForConnected(1000)) // thread 1 - fault wait
{
// enter. but switch executing context
code 1
}

if(!socket2->waitForConnected(1000)) // thread2 - fault wait
{
code 1
}

And now we have 2 (or more) threads that execute 'code1':

/-------------- thead 1 ----------------------------
auto server1 = new QLocalServer();
server1->removeServer("111");
server1->listen("111");

/-------------- thead 2 ----------------------------
auto server2 = new QLocalServer();
server2->removeServer("111");
server2->listen("111");
...

and of course we have 2 or more copies of the program running.

your solution is not working.

@kklimek
Copy link

kklimek commented May 7, 2015

@heretic13 were you able to reproduce the issue?

@itay-grudev
Copy link
Owner

I was able to. I will review the issue more carefully and think of a solution. Thanks!

@itay-grudev
Copy link
Owner

For reference, this is the code I used:

#include <QLocalServer>
#include <QLocalSocket>
#include <QThread>
#include <cstdio>

#define SERVER_NAME "111"

struct SocketOrServer : public QThread {
public:
    SocketOrServer(int threadNum) : QThread(), threadNum(threadNum) { }

private:
    void run()
    {
        printf("connecting\n");
        socket = new QLocalSocket();
        socket->connectToServer(SERVER_NAME);
        if( ! socket->waitForConnected(1000)) {
            delete socket;
            printf("creating server on thread %d\n", threadNum);
            server = new QLocalServer();
            server->removeServer(SERVER_NAME);
            server->listen(SERVER_NAME);
        }
    }

    int threadNum;
    QLocalSocket *socket;
    QLocalServer *server;

} client1(1), client2(2);

int main(int argc, char *argv[])
{
    client1.start();
    client2.start();
    client1.wait();
    client2.wait();
}

P.S. Though the class was intended to avoid a user starting your app twice, not a computer.

@kklimek
Copy link

kklimek commented May 7, 2015

@itay-grudev but with the "removeServer" lines? Because with them it's easy to reproduce the issue. Also it may be only Windows platform issue as the docs state: On Windows two local servers can listen to the same pipe at the same time, but any connections will go to one of the server.

@itay-grudev
Copy link
Owner

Though the class was intended to avoid a user starting your app twice, not a computer

@kklimek I am actually testing it under Unix, the code is above.

@heretic13
Copy link
Author

My goal was to find a singleton implementation for qt.
Your project I found on the links.

I also check here - http://habrahabr.ru/post/173281/
this is a singleton. good solution (for singleton)

but author have a mistake. They make attach memory then create - possible multithreading race.

Relly simple singleton - use

QSharedMemory mem1("111");
bool bAlreadyRunning=!mem1.create(1);

But author say that app-crashed on linux not cleanup QSharedMemory.

I think the truth (for singleton) is somewhere in the middle.

itay-grudev added a commit that referenced this issue May 7, 2015
This bug is related to issue #3. I expect to push a solution soon
@itay-grudev
Copy link
Owner

@heretic13 you are correct. I also thought that the solution is somewhere in the middle. I will investigate it and push a fix as soon as possible.

@kklimek
Copy link

kklimek commented May 7, 2015

I've came up with solution like this:

  1. check other instance with localsocket
  2. If no instance try to listen
  3. If listen fails check for the reason, if reason is AddressInUseError check other instance again
  4. If still no other instance call removeServer
  5. listen again

@itay-grudev itay-grudev added the bug label May 7, 2015
@itay-grudev itay-grudev self-assigned this May 7, 2015
itay-grudev added a commit that referenced this issue May 7, 2015
This bug is related to issue #3. I expect to push a solution soon
itay-grudev added a commit that referenced this issue May 7, 2015
This bug is related to issue #3. I expect to push a solution soon
@itay-grudev
Copy link
Owner

QSharedMemory might not be thread safe. In fact, unless you initialise it from a parent thread, QSharedMemory will initialise two instances of the shared memory, if two threads are running simultaneously.

@heretic13
Copy link
Author

"QSharedMemory is not thread safe."

Provide an example. I must see it.

@itay-grudev
Copy link
Owner

I have an idea. I can get around that. (Or it might have been that I misused QSharedMemory).

@itay-grudev
Copy link
Owner

This will work. I have a working prototype. I will integrate it in the library and I will push a commit later today with a fix. ; )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants