Check the primary connection and assume primary if no response is found.

[manyoso]

This guards against the case of stale shared memory that can happen when the primary application crashes or is forcefully killed.

Issue #190

[itay-grudev]

I get what you're trying to do here, and it's clever, but it creates a new issue. For a happy case where the primary is working correctly we end up triggering an instanceStarted signal twice because if the connection is successful, we write a message to it in connectToPrimary():

SingleApplication/singleapplication_p.cpp

Lines 267 to 286 in 494772e

	// Initialisation message according to the SingleApplication protocol
	QByteArray initMsg;
	QDataStream writeStream(&initMsg, QIODevice::WriteOnly);
	#if (QT_VERSION >= QT_VERSION_CHECK(5, 6, 0))
	writeStream.setVersion(QDataStream::Qt_5_6);
	#endif
	writeStream << blockServerName.toLatin1();
	writeStream << static_cast<quint8>(connectionType);
	writeStream << instanceNumber;
	#if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0)
	quint16 checksum = qChecksum(QByteArray(initMsg.constData(), static_cast<quint32>(initMsg.length())));
	#else
	quint16 checksum = qChecksum(initMsg.constData(), static_cast<quint32>(initMsg.length()));
	#endif
	writeStream << checksum;
	return writeConfirmedMessage( static_cast<int>(msecs - time.elapsed()), initMsg );
	}

I think this can be avoided if we pass an argument to connectToPrimary() so it can abort the connection early. Think something like:

We add a new ConnectionType:

    enum ConnectionType : quint8 {
        ...
        ConnectionTest = 4  
    };

We pass it to connectToPrimary:

if ( inst->primary != false && !d->connectToPrimary( timeout, SingleApplicationPrivate::ConnectionTest )) {

In connectToPrimary we can then abort early. Place this before the message initialization after the long if statement that ensures connection

if( connectionType == SingleApplicationPrivate::ConnectionTest ){
  socket->close();
  return true;
}

None of this is tested. I leave it to you for implementation details as I can't really spend time on this project right now. But of course PR's are most welcome.

[manyoso]

Ah, yeah, that'll have to be fixed, but otherwise how is this approach? Does it dissuade you from the wholesale rewrite trying to eliminate the QSharedMemory? Really, with this fix the project meets our needs pretty well.

[itay-grudev]

@manyoso I don't mind accepting your PR and I'll release it as v3.6.0. I'll dedicate a whole minor version to this, because it's a genuinely clever hack that covers a very important corner-case.

...but I still want to do the rewrite. It will be a good improvement and I'll do my best to keep the API backwards compatible. But it has to be done, since Qt changed the implementation behind the QSharedMemory it's extremely unreliable and that the primary method this library is keeping track of its state.

Most of my work is client driven though. That's how this project started in the first place. I had a client who needed a more flexible Single Application solution which they described as: "The way Skype works! Click it again - it opens the existing app". I then had another client which paid for the v3 rewrite.

Unfortunately I haven't had any new Qt projects to justify the v4 work and my focus has been elsewhere. So with just my free time I can support the project, but can't allocate the time it really needs.

[itay-grudev]

A QLocalSocket only solution (which is basically the direction you are going) is going to be better. But it should be written for it from the start.

Your patch will improve the status quo, but there are still problems. I need to run the QLocalServer in a separate thread with its own Event Loop. Otherwise if your main thread is busy all those connection attempts will timeout making secondary applications think they are primary.

That's kind of what the v4 branch is supposed to do, but I'm still not ready.

[manyoso]

Okay, thanks for letting me know. Just as an FYI we're planning to use this in GPT4All: https://github.com/nomic-ai/gpt4all :)