D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Oleg Solovyov]

McPain created this revision.
McPain added reviewers: broulik, ngraham.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
McPain requested review of this revision.

REVISION SUMMARY
  BUG: 397537
  
  Earlier plasmashell assumed that you'll get free space info immediately 
(which is not true in case of losing connection to server containing a mounted 
filesystem - statfs will wait for response forever and freeze everything since 
it's happening in main thread)
  
  I moved obtaining that info into different thread so that case won't freeze 
anything anymore.
  It creates exactly one thread per one path. If a path is already being 
processed, new thread won't be created.
  Also I implemented a timer used to notify about broken connection after 15 
seconds.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D14895

AFFECTED FILES
  dataengines/soliddevice/CMakeLists.txt
  dataengines/soliddevice/soliddeviceengine.cpp
  dataengines/soliddevice/soliddeviceengine.h

To: McPain, broulik, ngraham
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Oleg Solovyov]

McPain updated this revision to Diff 39897.
McPain added a comment.


  Removed useless debug string

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14895?vs=39896&id=39897

REVISION DETAIL
  https://phabricator.kde.org/D14895

AFFECTED FILES
  dataengines/soliddevice/CMakeLists.txt
  dataengines/soliddevice/soliddeviceengine.cpp
  dataengines/soliddevice/soliddeviceengine.h

To: McPain, broulik, ngraham
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[David Edmundson]

davidedmundson added a comment.


  Thanks for looking into this.
  
  Whilst this looks fine,  we have a more standard abstraction layer for async 
IO via KIO.
  There's a FileSystemFreeSpaceJob which should work here.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham
Cc: davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> soliddeviceengine.cpp:67
> +KDiskFreeSpaceInfo info = KDiskFreeSpaceInfo::freeSpaceInfo(m_path);
> +emit infoObtained(m_path, m_udi, info.isValid(), info.available(), 
> info.size());
> +}

Don't you forget to emit finished?

> soliddeviceengine.cpp:591-600
> +connect(thread, SIGNAL(started()), worker, SLOT(getFreeSpaceInfo()));
> +connect(worker, SIGNAL(finished()), thread, SLOT(quit()));
> +
> +// delete later thread and worker
> +connect(worker, SIGNAL(finished()), worker, SLOT(deleteLater()));
> +connect(thread, SIGNAL(finished()), thread, SLOT(deleteLater()));
> +

Use new connect syntax .

> soliddeviceengine.cpp:622
> +
> +m_timers[path]->deleteLater();
> +m_timers.remove(path);

It does not need, you connect finished to deleteLater.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Oleg Solovyov]

McPain updated this revision to Diff 39901.
McPain marked 3 inline comments as done.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14895?vs=39897&id=39901

REVISION DETAIL
  https://phabricator.kde.org/D14895

AFFECTED FILES
  dataengines/soliddevice/CMakeLists.txt
  dataengines/soliddevice/soliddeviceengine.cpp
  dataengines/soliddevice/soliddeviceengine.h

To: McPain, broulik, ngraham
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Oleg Solovyov]

McPain added a comment.


  In D14895#310535 , @davidedmundson 
wrote:
  
  > Thanks for looking into this.
  >
  > Whilst this looks fine,  we have a more standard abstraction layer for 
async IO via KIO.
  >  There's a FileSystemFreeSpaceJob which should work here.
  
  
  We can always do it later, fixibg bug itself is more important than making it 
through KIO

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added a comment.
This revision now requires changes to proceed.


  But the KIO stuff is already there.
  
  > We can always do it later
  
  That's never how it works.

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Kai Uwe Broulik]

broulik added a comment.


  Yes, please use `KIO::FileSystemFreeSpaceJob` which is a lot simpler than all 
of this manually added threading code.
  
  Also beware that `bool SolidDeviceEngine::updateSourceEvent` is currently 
supposed to return `true` for when any of the sources changed which will no 
longer work when `updateStorageSpace` becomes asynchronous. Might not be an 
issue as you call `setData` anyway but just something I want you to keep in 
mind for testing.

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Oleg Solovyov]

McPain added a comment.


  In D14895#310633 , @broulik wrote:
  
  > Also beware that `bool SolidDeviceEngine::updateSourceEvent` is currently 
supposed to return `true`
  
  
  Who will use that `true`?
  I didn't found any places where that value is used.
  
  The only place I found ignores it:
  
void SolidDeviceEngine::deviceChanged(const QString& udi, const QString 
&property, const QVariant &value)
{
setData(udi, property, value);
updateSourceEvent(udi);
}

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Oleg Solovyov]

McPain added a comment.


  In D14895#310633 , @broulik wrote:
  
  > Might not be an issue as you call `setData` anyway but just something I 
want you to keep in mind for testing.
  
  
  I

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Oleg Solovyov]

McPain added a comment.


  In D14895#310633 , @broulik wrote:
  
  > Might not be an issue as you call `setData` anyway but just something I 
want you to keep in mind for testing.
  
  
  something like
  
setData(udi, I18N_NOOP("Free Space"), QVariant(0));
setData(udi, I18N_NOOP("Free Space Text"), KFormat().formatByteSize(0));
setData(udi, I18N_NOOP("Size"), QVariant(0));
  
  or what?
  And what to do after obtaining **real** data?

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Kai Uwe Broulik]

broulik added a comment.


  It is just a question. I think since you call `setData` when the job finishes 
anyway, it should be fine but I don't really know the architecture of that 
stuff either, so it might be fine to just trigger a free space job in 
`updateSourceEvent`

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Oleg Solovyov]

McPain updated this revision to Diff 39929.
McPain added a comment.


  Used KIO::FileSystemFreeSpaceJob

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14895?vs=39901&id=39929

REVISION DETAIL
  https://phabricator.kde.org/D14895

AFFECTED FILES
  dataengines/soliddevice/CMakeLists.txt
  dataengines/soliddevice/soliddeviceengine.cpp
  dataengines/soliddevice/soliddeviceengine.h

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> soliddeviceengine.cpp:564
> +
> +m_timers[path] = timer;
> +

You can remove it, see below.

> soliddeviceengine.cpp:574
> +connect(job, &KIO::FileSystemFreeSpaceJob::result,
> +[this, path, udi](KIO::Job *job, KIO::filesize_t size, 
> KIO::filesize_t available) {
> +if (m_timers[path]->isActive())

Capture timer too

> soliddeviceengine.cpp:575-576
> +[this, path, udi](KIO::Job *job, KIO::filesize_t size, 
> KIO::filesize_t available) {
> +if (m_timers[path]->isActive())
> +m_timers[path]->stop();
> +

You can omit declaration of map, just

  timer->stop();

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> soliddeviceengine.cpp:588
> +// start timer: after 15 seconds we will get an error
> +timer->start(15000);
>  }

job->start();

as well.

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Oleg Solovyov]

McPain added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in soliddeviceengine.cpp:564
> You can remove it, see below.

What if it creates another job working on the same path?

The purpose is "check if nobody is working on path", not "save the timer 
pointer"

> anthonyfieroni wrote in soliddeviceengine.cpp:588
>   job->start();
> 
> as well.

Job starts without it

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Oleg Solovyov]

McPain updated this revision to Diff 39933.
McPain marked 4 inline comments as done.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14895?vs=39929&id=39933

REVISION DETAIL
  https://phabricator.kde.org/D14895

AFFECTED FILES
  dataengines/soliddevice/CMakeLists.txt
  dataengines/soliddevice/soliddeviceengine.cpp
  dataengines/soliddevice/soliddeviceengine.h

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Oleg Solovyov]

McPain updated this revision to Diff 39934.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14895?vs=39933&id=39934

REVISION DETAIL
  https://phabricator.kde.org/D14895

AFFECTED FILES
  dataengines/soliddevice/CMakeLists.txt
  dataengines/soliddevice/soliddeviceengine.cpp
  dataengines/soliddevice/soliddeviceengine.h

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[David Edmundson]

davidedmundson added a comment.


  Much nicer! I'm definitely happy merging something like this.

INLINE COMMENTS

> soliddeviceengine.cpp:555
> +if (!m_paths.contains(path)) {
> +QTimer *timer = new QTimer(this);
> +timer->setSingleShot(true);

not sure you need this timer

1. You should still get the job finishing with error ERR_SERVER_TIMEOUT. 
(untested, but the protocol manager in the client does have some stuff doing 
this)

2. if you don't get a progress bar for a drive that's frozen anyway, do you 
really care?

> soliddeviceengine.cpp:560
> + i18n("Filesystem mounted at '%1' is not 
> responding", path),
> + QApplication::activeWindow(), 
> KNotification::CloseWhenWidgetActivated);
> +});

We don't know which window will be active when this timer fires. Which means 
the behavior to dismiss is somewhat random.

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Oleg Solovyov]

McPain added inline comments.

INLINE COMMENTS

> davidedmundson wrote in soliddeviceengine.cpp:555
> not sure you need this timer
> 
> 1. You should still get the job finishing with error ERR_SERVER_TIMEOUT. 
> (untested, but the protocol manager in the client does have some stuff doing 
> this)
> 
> 2. if you don't get a progress bar for a drive that's frozen anyway, do you 
> really care?

There are solutions to notify user about stuck FS without that timer, right?

> davidedmundson wrote in soliddeviceengine.cpp:560
> We don't know which window will be active when this timer fires. Which means 
> the behavior to dismiss is somewhat random.

I tested it - works for me.

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> McPain wrote in soliddeviceengine.cpp:555
> There are solutions to notify user about stuck FS without that timer, right?

I think you can just move the notification to

connect(job, &KIO::FileSystemFreeSpaceJob::result, ...) {

if (job->error() == ERR_SERVER_TIMEOUT) {

  

}
}

but I haven't tested that.

> McPain wrote in soliddeviceengine.cpp:560
> I tested it - works for me.

Sure, it /might/ work, but you're still fetching a random window N  seconds 
after a user event.

I don't see how we can know that will result in correct behaviour.

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Oleg Solovyov]

McPain added inline comments.

INLINE COMMENTS

> davidedmundson wrote in soliddeviceengine.cpp:560
> Sure, it /might/ work, but you're still fetching a random window N  seconds 
> after a user event.
> 
> I don't see how we can know that will result in correct behaviour.

Any ideas how to do it correctly?

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> McPain wrote in soliddeviceengine.cpp:560
> Any ideas how to do it correctly?

It has a default parameters just call event with 3 parameters not 5.

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Oleg Solovyov]

McPain added inline comments.

INLINE COMMENTS

> davidedmundson wrote in soliddeviceengine.cpp:555
> I think you can just move the notification to
> 
> connect(job, &KIO::FileSystemFreeSpaceJob::result, ...) {
> 
> if (job->error() == ERR_SERVER_TIMEOUT) {
> 
>   
> 
> }
> }
> 
> but I haven't tested that.

I did. Job will never finish in that case

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Oleg Solovyov]

McPain updated this revision to Diff 40923.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14895?vs=39934&id=40923

REVISION DETAIL
  https://phabricator.kde.org/D14895

AFFECTED FILES
  dataengines/soliddevice/CMakeLists.txt
  dataengines/soliddevice/soliddeviceengine.cpp
  dataengines/soliddevice/soliddeviceengine.h

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Kai Uwe Broulik]

broulik added a comment.


  I'm not a huge fan of having that notification but if you can assure it 
doesn't spam the user by showing up repeatedly if a mount is blocked 
indefinitely, it's fine I guess

INLINE COMMENTS

> soliddeviceengine.cpp:29
>  #include 
>  #include 
>  #include 

Unused

> soliddeviceengine.cpp:571
> +// collect and process info
> +connect(job, &KIO::FileSystemFreeSpaceJob::result,
> +[this, timer, path, udi](KIO::Job *job, KIO::filesize_t 
> size, KIO::filesize_t available) {

Give `connect` a context:

  connect(job, &KIO::FileSystemFreeSpaceJob::result, this, ...

otherwise it would blow up if we are deleted before the job finishes

> soliddeviceengine.h:38
>  #include "hddtemp.h"
> +#include 
> +#include 

Unused?

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Oleg Solovyov]

McPain updated this revision to Diff 40973.
McPain marked 12 inline comments as done.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14895?vs=40923&id=40973

REVISION DETAIL
  https://phabricator.kde.org/D14895

AFFECTED FILES
  dataengines/soliddevice/CMakeLists.txt
  dataengines/soliddevice/soliddeviceengine.cpp
  dataengines/soliddevice/soliddeviceengine.h

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Oleg Solovyov]

McPain added a comment.


  Just in case, I don't have commit access

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Nathaniel Graham]

ngraham added a comment.


  Does this look good now @davidedmundson?

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham, davidedmundson
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[David Edmundson]

davidedmundson resigned from this revision.
davidedmundson added a comment.
This revision is now accepted and ready to land.


  I wouldn't have kept the notification myself, but its fine. Ship it.

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik, ngraham
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Nathaniel Graham]

ngraham resigned from this revision.
ngraham added a comment.


  +1 for the concept. No opinion about the notification, but I think it's fine. 
Removing myself as a reviewer since I don't feel qualified to offer a code 
review. But sounds like you've got a shipit! You need someone to land it for 
you, right?

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik
Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Oleg Solovyov]

McPain added a subscriber: ngraham.
McPain added a comment.


  In D14895#320987 , @ngraham wrote:
  
  > +1 for the concept. No opinion about the notification, but I think it's 
fine. Removing myself as a reviewer since I don't feel qualified to offer a 
code review. But sounds like you've got a shipit! You need someone to land it 
for you, right?
  
  
  Right. Nobody gave me a commit access yet

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik
Cc: ngraham, anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:e1c19ce4daf9: Plasmashell freezes when trying to get free 
space info from mounted remote… (authored by McPain, committed by ngraham).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14895?vs=40973&id=41511

REVISION DETAIL
  https://phabricator.kde.org/D14895

AFFECTED FILES
  dataengines/soliddevice/CMakeLists.txt
  dataengines/soliddevice/soliddeviceengine.cpp
  dataengines/soliddevice/soliddeviceengine.h

To: McPain, broulik
Cc: ngraham, anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Alan Justino da Silva]

alanjds added a comment.


  As described on https://bugs.kde.org/show_bug.cgi?id=401088, this 
notification is triggering when it should not: during short bursts of high CPU 
load, instead of when a real issue is ongoing.
  
  Then the //handling// of the high amount of notifications is extending the 
CPU load, possibly creating more notifications in a cycle.
  
  Not saying to remove the feature, but I would benefit from having the default 
timeout of 15 seconds configurable.
  If easy, I can contribute the commit, given some guiding examples. Yet I just 
want this not locking my machine 😬.

INLINE COMMENTS

> soliddeviceengine.cpp:583-584
> +
> +// start timer: after 15 seconds we will get an error
> +timer->start(15000);
>  }

Can we have this number configurable somehow, even if only before startup.

It's been annoying to have notifications when bringing lots of Docker instances 
at once.

The overhead of _receiving and showing_ the notifications is being longer than 
the one about bringing the instances up.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik
Cc: alanjds, ngraham, anthonyfieroni, davidedmundson, plasma-devel, balaji, 
galderaa, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Méven Car]

meven added a comment.


  Feel free to send a MR, and include more context please.

INLINE COMMENTS

> alanjds wrote in soliddeviceengine.cpp:583-584
> Can we have this number configurable somehow, even if only before startup.
> 
> It's been annoying to have notifications when bringing lots of Docker 
> instances at once.
> 
> The overhead of _receiving and showing_ the notifications is being longer 
> than the one about bringing the instances up.

Better approach would be to make this check ignore docker overlay fs.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik
Cc: meven, alanjds, ngraham, anthonyfieroni, davidedmundson, plasma-devel, 
balaji, galderaa, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Alan Justino da Silva]

alanjds added a comment.


  I like the idea of making docker imune. May be improved.

INLINE COMMENTS

> meven wrote in soliddeviceengine.cpp:583-584
> Better approach would be to make this check ignore docker overlay fs.

That would fix my issue, sure. Idk if targeting "docker" is better than having 
it configurable, as other similar solutions will have the same problem.

I liked the idea of making docker imune here. May be even better by having a 
passlist of what should be imune, allowing the user to address issues for 
similar solutions

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik
Cc: meven, alanjds, ngraham, anthonyfieroni, davidedmundson, plasma-devel, 
balaji, galderaa, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Alan Justino da Silva]

alanjds added a comment.


  In D14895#683516 , @meven wrote:
  
  > Feel free to send a MR, and include more context please.
  
  
  Sorry my ignorance. What "MR" stands for, in this context?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik
Cc: meven, alanjds, ngraham, anthonyfieroni, davidedmundson, plasma-devel, 
balaji, galderaa, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

[Méven Car]

meven added a comment.


  In D14895#683521 , @alanjds wrote:
  
  > In D14895#683516 , @meven wrote:
  >
  > > Feel free to send a MR, and include more context please.
  >
  >
  > Sorry my ignorance. What "MR" stands for, in this context?
  
  
  You are more than welcome to ask that's jargon.
  MR stands for Merge Request (Gitlab's PR).

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D14895

To: McPain, broulik
Cc: meven, alanjds, ngraham, anthonyfieroni, davidedmundson, plasma-devel, 
balaji, galderaa, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart