Martin Koller | 14 Jan 19:38 2012
Picon

Review Request: Avoid QDBusConnection Qt warning message for each KUniqueApplication

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103699/

Review request for kdelibs.
By Martin Koller.

Description

All KUniqueApplications issue the warning QDBusConnection: session D-Bus connection created before QCoreApplication. Application may misbehave. when runngin with Qt-4.8.0 (qWarning in QDBusDefaultConnection ctor) The patch avoids this by temporarily creating a QCoreApplication instance

Testing

running kdepasswd

Diffs

  • kdeui/kernel/kuniqueapplication.cpp (777fc35)

View Diff

Thiago Macieira | 14 Jan 22:36 2012
Picon

Re: Review Request: Avoid QDBusConnection Qt warning message for each KUniqueApplication

On Saturday, 14 de January de 2012 18.38.02, Martin Koller wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103699/
> -----------------------------------------------------------
> 
> Review request for kdelibs.
> 
> 
> Description
> -------
> 
> All KUniqueApplications issue the warning
> QDBusConnection: session D-Bus connection created before QCoreApplication.
> Application may misbehave. when runngin with Qt-4.8.0 (qWarning in
> QDBusDefaultConnection ctor)
> 
> The patch avoids this by temporarily creating a QCoreApplication instance

If this is done before the fork, it's a REALLY BAD IDEA.

If it's done after the fork, why is the connection created this early? I might 
have written this code, but I certainly don't remember it anymore.

--

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
      PGP/GPG: 0x6EF45358; fingerprint:
      E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358
Martin Koller | 15 Jan 11:47 2012
X-Face
Picon

Re: Review Request: Avoid QDBusConnection Qt warning message for each KUniqueApplication

On Saturday, 14. January 2012 22:36:44 Thiago Macieira wrote:
> On Saturday, 14 de January de 2012 18.38.02, Martin Koller wrote:
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > http://git.reviewboard.kde.org/r/103699/
> > -----------------------------------------------------------
> > 
> > Review request for kdelibs.
> > 
> > 
> > Description
> > -------
> > 
> > All KUniqueApplications issue the warning
> > QDBusConnection: session D-Bus connection created before QCoreApplication.
> > Application may misbehave. when runngin with Qt-4.8.0 (qWarning in
> > QDBusDefaultConnection ctor)
> > 
> > The patch avoids this by temporarily creating a QCoreApplication instance
> 
> If this is done before the fork, it's a REALLY BAD IDEA.

it is done after the fork (if there is a fork and not used with --nofork).

> If it's done after the fork, why is the connection created this early? I might 
> have written this code, but I certainly don't remember it anymore.

According to the header doc KUniqueApplication shall be used like this:

...
   *    if (!KUniqueApplication::start()) {
   *       fprintf(stderr, "myAppName is already running!\n");
   *       return 0;
   *    }
   *    KUniqueApplication a;
   *    return a.exec();

So one of the very first things the static start() method does is to
open the session dbus, and this is before a QCoreApplication is available.

So my idea here was to create one and delete it afterwards, which should
result in the same state as before the call to tryToInitDBusConnection()

If that is a bad idea, i'd like to learn why and what can be done otherwise.
(One other workaround to get rid of the warning message would be to set up
an own message handler to discard this message, but I'd rather not do that...)

--

-- 
Best regards/Schöne Grüße

Martin
A: Because it breaks the logical sequence of discussion
Q: Why is top posting bad?

()  ascii ribbon campaign - against html e-mail 
/\  www.asciiribbon.org   - against proprietary attachments

Geschenkideen, Accessoires, Seifen, Kulinarisches: www.bibibest.at
Martin Koller | 15 Jan 13:11 2012
X-Face
Picon

Re: Review Request: Avoid QDBusConnection Qt warning message for each KUniqueApplication

On Sunday, 15. January 2012 11:47:10 Martin Koller wrote:
> On Saturday, 14. January 2012 22:36:44 Thiago Macieira wrote:
> > On Saturday, 14 de January de 2012 18.38.02, Martin Koller wrote:
> > > -----------------------------------------------------------
> > > This is an automatically generated e-mail. To reply, visit:
> > > http://git.reviewboard.kde.org/r/103699/
> > > -----------------------------------------------------------
> > > 
> > > Review request for kdelibs.
> > > 
> > > 
> > > Description
> > > -------
> > > 
> > > All KUniqueApplications issue the warning
> > > QDBusConnection: session D-Bus connection created before QCoreApplication.
> > > Application may misbehave. when runngin with Qt-4.8.0 (qWarning in
> > > QDBusDefaultConnection ctor)
> > > 
> > > The patch avoids this by temporarily creating a QCoreApplication instance
> > 
> > If this is done before the fork, it's a REALLY BAD IDEA.
> 
> it is done after the fork (if there is a fork and not used with --nofork).
> 
> > If it's done after the fork, why is the connection created this early? I might 
> > have written this code, but I certainly don't remember it anymore.
> 
> According to the header doc KUniqueApplication shall be used like this:
> 
> ...
>    *    if (!KUniqueApplication::start()) {
>    *       fprintf(stderr, "myAppName is already running!\n");
>    *       return 0;
>    *    }
>    *    KUniqueApplication a;
>    *    return a.exec();
> 
> So one of the very first things the static start() method does is to
> open the session dbus, and this is before a QCoreApplication is available.
> 
> So my idea here was to create one and delete it afterwards, which should
> result in the same state as before the call to tryToInitDBusConnection()
> 
> If that is a bad idea, i'd like to learn why and what can be done otherwise.
> (One other workaround to get rid of the warning message would be to set up
> an own message handler to discard this message, but I'd rather not do that...)

ok, I see why this is a bad idea: The Qt doc says:

"... the sessionBus() and systemBus() ...
Those connections are opened when first used and are closed when the QCoreApplication destructor is run."

so it seems the return value of tryToInitDBusConnection() is no longer valid when the QCoreApplication
was destroyed before ...

Hmmm... When the check for uniqueness needs a coreapplication object, but KUniqueApplication
is designed in this way to create the coreapplication after checking ... this is a problem.

It seems creating and deleting QCoreApplication can really only be done once in a process ...
Any idea how to solve this ?

--

-- 
Best regards/Schöne Grüße

Martin
A: Because it breaks the logical sequence of discussion
Q: Why is top posting bad?

()  ascii ribbon campaign - against html e-mail 
/\  www.asciiribbon.org   - against proprietary attachments

Geschenkideen, Accessoires, Seifen, Kulinarisches: www.bibibest.at
Thiago Macieira | 15 Jan 13:18 2012
Picon

Re: Review Request: Avoid QDBusConnection Qt warning message for each KUniqueApplication

On Sunday, 15 de January de 2012 11.47.10, Martin Koller wrote:
>    *    if (!KUniqueApplication::start()) {
>    *       fprintf(stderr, "myAppName is already running!\n");
>    *       return 0;
>    *    }
>    *    KUniqueApplication a;
>    *    return a.exec();
> 
> So one of the very first things the static start() method does is to
> open the session dbus, and this is before a QCoreApplication is available.

It should open a session bus connection, but not *the* 
QDBusConnection::sessionBus() connection.

I know I designed the code like that. You can't open it before because that 
would mean the file descriptor is opened in the wrong process. So where is the 
warninig coming from? Did I place the warning in the wrong object in QtDBus? 
Or is there something *else* creating that connection?

Your description still requires the QCoreApplication object to be created 
before the fork. It doesn't matter that you're going to close that open and 
open a new one after the fork: it's still a bad idea. Some Unix operations do 
not survive forks, like creation of threads. If you do that, I'm pretty sure 
QProcess will be irreparably broken.
--

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
      PGP/GPG: 0x6EF45358; fingerprint:
      E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358
Martin Koller | 15 Jan 18:28 2012
X-Face
Picon

Re: Review Request: Avoid QDBusConnection Qt warning message for each KUniqueApplication

On Sunday, 15. January 2012 13:18:58 Thiago Macieira wrote:
> On Sunday, 15 de January de 2012 11.47.10, Martin Koller wrote:
> >    *    if (!KUniqueApplication::start()) {
> >    *       fprintf(stderr, "myAppName is already running!\n");
> >    *       return 0;
> >    *    }
> >    *    KUniqueApplication a;
> >    *    return a.exec();
> > 
> > So one of the very first things the static start() method does is to
> > open the session dbus, and this is before a QCoreApplication is available.
> 
> It should open a session bus connection, but not *the* 
> QDBusConnection::sessionBus() connection.
> 
> I know I designed the code like that. You can't open it before because that 
> would mean the file descriptor is opened in the wrong process. So where is the 
> warninig coming from?

qWarning is in QDBusDefaultConnection ctor

> Did I place the warning in the wrong object in QtDBus? 
> Or is there something *else* creating that connection?

no, it's the call to QDBusConnection::sessionBus()
which creates the QDBusDefaultConnection object which throws that warning.

> Your description still requires the QCoreApplication object to be created 
> before the fork. It doesn't matter that you're going to close that open and 
> open a new one after the fork: it's still a bad idea. Some Unix operations do 
> not survive forks, like creation of threads. If you do that, I'm pretty sure 
> QProcess will be irreparably broken.

Sorry, I don't understand that.
Nevertheless, I tried now what you've suggested above, namely to create my own
QDBusConnection object to the session bus. This seems to work now, however
as I'm new to DBus, I do not fully understand everything.
What I've found out is that the KUniqueApplication's DBus call to /MainApplication
only works if I create a QDBusConnection object on the session bus only if it
uses the same name as the Qt-internal QDBusDefaultConnection object, which is
"qt_default_session_bus", even when I would create my QDBusConnection objects
in the way that they live as long as the app lives.
Does that mean that a registered Object on DBus is linked to the QDBusConnection's name ?

I've updated the diff https://git.reviewboard.kde.org/r/103699/diff/2/
--

-- 
Best regards/Schöne Grüße

Martin
A: Because it breaks the logical sequence of discussion
Q: Why is top posting bad?

()  ascii ribbon campaign - against html e-mail 
/\  www.asciiribbon.org   - against proprietary attachments

Geschenkideen, Accessoires, Seifen, Kulinarisches: www.bibibest.at
Thiago Macieira | 16 Jan 02:02 2012
Picon

Re: Review Request: Avoid QDBusConnection Qt warning message for each KUniqueApplication

On Sunday, 15 de January de 2012 18.28.28, Martin Koller wrote:
> qWarning is in QDBusDefaultConnection ctor
> 
> > Did I place the warning in the wrong object in QtDBus?
> > Or is there something *else* creating that connection?
> 
> no, it's the call to QDBusConnection::sessionBus()
> which creates the QDBusDefaultConnection object which throws that warning.

I know. My question is: why was QDBusConnection::sessionBus() called before 
QCoreApplication was created? Can't we fix *that* by initialising it later?

> > Your description still requires the QCoreApplication object to be created
> > before the fork. It doesn't matter that you're going to close that open
> > and
> > open a new one after the fork: it's still a bad idea. Some Unix operations
> > do not survive forks, like creation of threads. If you do that, I'm
> > pretty sure QProcess will be irreparably broken.
> 
> Sorry, I don't understand that.
> Nevertheless, I tried now what you've suggested above, namely to create my
> own QDBusConnection object to the session bus. This seems to work now,
> however as I'm new to DBus, I do not fully understand everything.
> What I've found out is that the KUniqueApplication's DBus call to
> /MainApplication only works if I create a QDBusConnection object on the
> session bus only if it uses the same name as the Qt-internal
> QDBusDefaultConnection object, which is "qt_default_session_bus", even when
> I would create my QDBusConnection objects in the way that they live as long
> as the app lives.
> Does that mean that a registered Object on DBus is linked to the
> QDBusConnection's name ?

No, it means you really did not understand the code you're trying to change.

Here's *should* go on:

1) KUniqueApplication connects to D-Bus using a private connection

2) it checks if the application is already running. If it is and we would 
fork, we simply ask the running instance to show a new main window. Then we 
exit.

3) if it wasn't running, it forks. The parent process waits for the child 
process to register itself on the bus using the same private connection. The 
child process closes its end of the private connection.

4) the child process initialises the QCoreApplication object

5) the child process opens the default, shared session bus connection and 
registers itself on the bus

6) the parent process notices the child is up and running, so it exits with 
_exit(0) (not exit(0)).

You're getting the warning because steps 4 and 5 are inverted. Your patch 
should de-invert them by making the very first call to 
QDBusConnection::sessionBus() happen after the QCoreApplication's constructor 
has finished running. That means it can run in the KApplication constructor 
body or in the KUniqueApplication constructor body, but not before.

> I've updated the diff https://git.reviewboard.kde.org/r/103699/diff/2/

I can't find my password. Someone convinced me a while ago to use passwords 
generated with keepassx, especially because the KDE accounts kept getting 
locked because of passwords being too easy to crack.

Can you send the patch?

--

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
      PGP/GPG: 0x6EF45358; fingerprint:
      E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358
Martin Koller | 15 Jan 18:28 2012
Picon

Re: Review Request: Avoid QDBusConnection Qt warning message for each KUniqueApplication

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103699/

Review request for kdelibs.
By Martin Koller.

Updated Jan. 15, 2012, 5:28 p.m.

Changes

Don't create QCoreApplication but an own session bus QDBusConnection object.

Description

All KUniqueApplications issue the warning QDBusConnection: session D-Bus connection created before QCoreApplication. Application may misbehave. when runngin with Qt-4.8.0 (qWarning in QDBusDefaultConnection ctor) The patch avoids this by temporarily creating a QCoreApplication instance

Testing

running kdepasswd

Diffs (updated)

  • kdeui/kernel/kuniqueapplication.cpp (777fc35)

View Diff

David Faure | 5 Mar 16:33 2012
Picon

Re: Review Request: Avoid QDBusConnection Qt warning message for each KUniqueApplication

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103699/

Looks ok to me, except for a few details. I presume you tested all combinations? The kuniqueapplication unit test, the new app case, the already-running app case, the --nofork case?
kdeui/kernel/kuniqueapplication.cpp (Diff revision 2) 99 99
KUniqueApplication::addCmdLineOptions()
QDBusConnection sessionBus = QDBusConnection::sessionBus(); : QDBusConnection(QDBusConnection::connectToBus(QDBusConnection::SessionBus, "qt_default_session_bus"))
Is there a reason for this particular connection name? Seems to match one from Qt itself, can you add a comment about why this is wanted?

kdeui/kernel/kuniqueapplication.cpp (Diff revision 2) 102 101
KUniqueApplication::addCmdLineOptions()
kError() << "KUniqueApplication: Cannot find the D-Bus session server: " << sessionBus.lastError().message() << endl; if ( !isConnected() || !interface() )
Qt/kdelibs coding style (for new code) is "no spaces inside parenthesis, opening brace on the same line".

kdeui/kernel/kuniqueapplication.cpp (Diff revision 2) 182 185
KUniqueApplication::start(StartFlags flags)
kError() << "KUniqueApplication: pipe() failed!" << endl; kError() << KCmdLineArgs::aboutData()->appName() << "KUniqueApplication: pipe() failed!";
Good idea, but please put the app name in a local var at the beginning of the method. It will reduce code duplication (I know it's not much code, but in kde frameworks 5 this will probably not come from KCmdLineArgs anymore).

- David


On January 15th, 2012, 5:28 p.m., Martin Koller wrote:

Review request for kdelibs.
By Martin Koller.

Updated Jan. 15, 2012, 5:28 p.m.

Description

All KUniqueApplications issue the warning QDBusConnection: session D-Bus connection created before QCoreApplication. Application may misbehave. when runngin with Qt-4.8.0 (qWarning in QDBusDefaultConnection ctor) The patch avoids this by temporarily creating a QCoreApplication instance

Testing

running kdepasswd

Diffs

  • kdeui/kernel/kuniqueapplication.cpp (777fc35)

View Diff

Steven Sroka | 16 Jun 01:38 2012
Picon

Re: Review Request: Avoid QDBusConnection Qt warning message for each KUniqueApplication

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103699/

Martin, what's the status of this patch? I think a bunch of bug reports on bko are waiting to get closed ;)

- Steven


On January 15th, 2012, 5:28 p.m., Martin Koller wrote:

Review request for kdelibs.
By Martin Koller.

Updated Jan. 15, 2012, 5:28 p.m.

Description

All KUniqueApplications issue the warning QDBusConnection: session D-Bus connection created before QCoreApplication. Application may misbehave. when runngin with Qt-4.8.0 (qWarning in QDBusDefaultConnection ctor) The patch avoids this by temporarily creating a QCoreApplication instance

Testing

running kdepasswd

Diffs

  • kdeui/kernel/kuniqueapplication.cpp (777fc35)

View Diff

Martin Koller | 16 Jun 13:36 2012
Picon

Re: Review Request: Avoid QDBusConnection Qt warning message for each KUniqueApplication

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103699/

On June 15th, 2012, 11:38 p.m., Steven Sroka wrote:

Martin, what's the status of this patch? I think a bunch of bug reports on bko are waiting to get closed ;)
There was a mail thread on core-devel for this and Thiago Macieira said it's the wrong approach. I'll remove the patch ...

- Martin


On January 15th, 2012, 5:28 p.m., Martin Koller wrote:

Review request for kdelibs.
By Martin Koller.

Updated Jan. 15, 2012, 5:28 p.m.

Description

All KUniqueApplications issue the warning QDBusConnection: session D-Bus connection created before QCoreApplication. Application may misbehave. when runngin with Qt-4.8.0 (qWarning in QDBusDefaultConnection ctor) The patch avoids this by temporarily creating a QCoreApplication instance

Testing

running kdepasswd

Diffs

  • kdeui/kernel/kuniqueapplication.cpp (777fc35)

View Diff

David Faure | 18 Jun 14:10 2012
Picon

Re: Review Request: Avoid QDBusConnection Qt warning message for each KUniqueApplication

This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103699/

For the longer term, this problem is fixed in KDE Frameworks 5, by the replacement of KUniqueApplication with KDBusService, (which doesn't fork).

- David


On January 15th, 2012, 5:28 p.m., Martin Koller wrote:

Review request for kdelibs.
By Martin Koller.

Updated Jan. 15, 2012, 5:28 p.m.

Description

All KUniqueApplications issue the warning QDBusConnection: session D-Bus connection created before QCoreApplication. Application may misbehave. when runngin with Qt-4.8.0 (qWarning in QDBusDefaultConnection ctor) The patch avoids this by temporarily creating a QCoreApplication instance

Testing

running kdepasswd

Diffs

  • kdeui/kernel/kuniqueapplication.cpp (777fc35)

View Diff


Gmane