0

Consider this very simple example:

for(;;){
   QFile *file = new QFile("some.txt");//this is where it fails with "device not open" when windows screen is locked
   if(file->exists()){
      file->open(QFile::ReadOnly);
      QString contents = file->readAll();
      file->close();
   }
   file->deleteLater();
   QThread::sleep(2);
}

It was continuing to work as normal when windows screen is locked up until recently but I'm not sure if it started to act like this because I started to use Qt 5.9 or if it was a windows update that prevent file accesses while windows screen is locked.

So please advise a workaround or a solution. Thank you.

Edit: It turns out that QFile or file access was not the issue and the problem was where and by whom it was called. So I'm accepting @Kuba 's answer since it was informative and in the right direction.

Emre Beşirik
  • 57
  • 1
  • 11
  • Is it a specific file or this happen for any file? – vahancho Dec 05 '17 at 12:50
  • This code will hardly compile (file should be a pointer since you assign with new). – p-a-o-l-o Dec 05 '17 at 13:08
  • Sorry but this was written in here just for demonstration, please ignore semantic errors. and yes @vahancho any file. also please note that this is running in user space and is not running as a service. – Emre Beşirik Dec 05 '17 at 13:12
  • What do you mean by "it fails" at `new QFile()`? Does it throw an exception? – SteakOverflow Dec 05 '17 at 13:54
  • by "ti fails" I mean it outputs this message to console ""device not open" and all the rest fails because it considers it does not exist even though it does. if you put some debug messages in appropiate places and run this little code you'll see that while windows is logged-on it works fine and if you lock screen and log back in you'll see that it was failing while screen was locked. Or at least it does like that in my code. – Emre Beşirik Dec 05 '17 at 13:59
  • How did you identify the line `QFile *file = new QFile("some.txt");` as the one causing the problem? The "device not open" message is generally the result of a failed read or write and really shouldn't occur as the result of simply instantiating a `QFile`. – G.M. Dec 05 '17 at 15:15
  • I put debug prints in certain places to see where it fails – Emre Beşirik Dec 05 '17 at 16:00

1 Answers1

0

Your code leaks memory and resources: the QFile instances will never be destroyed, since you never let the event loop run in the current thread, and it's the event loop that would destroy them.

There's no reason to create the file object on the heap, and neither is there any reason to explicitly close the file: QFile is a proper C++ class that implements RAII. Destroying it is safe in any state, without resource leaks. Just destroy the file to close its underlying handle.

Incorporating the above suggestions, the code should be:

for (;;){
   QFile file("some.txt");
   if (file.exists()){
      file.open(QFile::ReadOnly);
      auto contents = file->readAll();
      qDebug() << "read, n=" << contents.size();
   }
   QThread::sleep(2);
}

Ideally, though, you should run your code from the event loop and not block the thread. Threads are expensive resources and having a thread that does nothing most of the time is rather wasteful.

auto timer = new QTimer;
timer->start(2000);
auto reader = []{
  QFile file("some.txt");
  if (file.exists()){
    file.open(QFile::ReadOnly);
    auto contents = file->readAll();
    qDebug() << "read, n=" << contents.size();
  }
});
connect(timer, &QTimer::timeout, reader);

To end the reading, stop or destroy the timer.

The thread should be spinning an event loop - and the default implementation of QThread::run will do that for you. Thus the code above can be wrapped in a QObject and moved to a dedicated thread, that should also be used for other things.

Alternatively, you could use the thread pool and do the reading in a worker thread, but run the timer in the GUI thread. Only the connect statement above would need to be changed to:

connect(timer, &QTimer::timeout, [reader]{ QtConcurrent::run(reader); });
Kuba hasn't forgotten Monica
  • 95,931
  • 16
  • 151
  • 313
  • Thank you for your detailed and enlighting answer but that little code snippet was just written in here only to be a base for explaining my problem easier. So it will never be used ofcourse but enough to test a simple problem. Thats all... But you’ve missed the all point of my problem. It would really be nice if you could comment on that part too. – Emre Beşirik Dec 05 '17 at 16:25
  • @EmreBeşirik: I did comment on it. To even begin any discussion of "my code doesn't fix it" you have to make an effort to actually **run my code** and create a test case that is **self contained** and demonstrates the issue. Otherwise any further discussions are pointless. Convert your code to an example that can be compiled in isolation. It's trivial. For inspiration, look at: https://github.com/KubaO/stackoverflown/tree/master/questions – Kuba hasn't forgotten Monica Dec 05 '17 at 16:29
  • Don’t get me wrong I really appreciate your efford and I would gladly run your code but the thing is that in my actual code the very same piece of code runs perfectly fine while windows user is logged in and fails when screen is locked. The only difference that maybe making any difference is that I’m calling it from a callback function (EnumDisplayMonitors). – Emre Beşirik Dec 05 '17 at 16:45
  • But you may have a point on running it on a different thread though. Since original was being called from a callback function the problem maybe is thread related – Emre Beşirik Dec 05 '17 at 16:50
  • The fundamental problem in your line of thinking is that you assume the answer. By assuming that the callback is irrelevant, you assume that the answer is that the callback indeed **is** irrelevant. Had you known that, you wouldn't be asking the question to start with though. So you're shooting yourself in the foot. Please create a complete self contained example. It will be ~100 lines long at most. By not doing that, you're not only contributing to the barrage of low quality questions on SO, but you're wasting your time. And our time, too. You don't know whether I "have a point". Find it out! – Kuba hasn't forgotten Monica Dec 05 '17 at 16:54