Error checking in juce_fileRead


#1

I’ve run into a situation where juce doesn’t seem to be able to tell the difference between an empty properties file and one that’s unreadable. This is a drag because I can’t tell whether a file contains a particular property or not. Something like:

PropertiesFile *foo = new PropertiesFile(unreadable_file_that_contains_property_bar,-1,PropertiesFile::storeAsXML); if (foo->containsKey("bar")) { // Do something } From looking through the code, it appears the root cause of this is that juce_fileRead doesn’t check the return value of ReadFile on windows. I can’t tell if the signature of juce_fileRead needs to change. juce_posix_SharedCode.h’s implementation of juce_fileRead does return what read() returns, so if callers that check the return value of juce_fileRead for -1 can learn about errors.

So maybe it’s enough to change juce_fileRead in juce_win32_Files.cpp to return -1 if ReadFile fails?

Though to get the info all the outside juce, we’d need more changes:

  • FileInputStream::read checks the return value of juce_fileRead for < 0 and then either throws an exception or somehow communicates the error to the caller (signature change?). Given the number of places that would need to change, an exception seems like the way to go.

Assuming an exception, I might not need any other changes. I can wrap the call the the PropertiesFile constructor in a try/catch block and I think I’m all set.

If there’s a decent chance a patch like this would get applied, I’ll write it.

Thanks.

-DB


#2

At some point in the future I would like to move to using exceptions for file errors, but don’t want to throw that in just yet.

Surely all you really need is for the propertiesfile to have a flag called isValidFile() that is set if it reads some valid xml or data? I don’t think that’d require deep changes to the file system functions(?)


#3

isValidFile works for me most of the time. I have run into a couple of snags though.

I can make the assert on line 129 of juce_PropertiesFile.cpp fire. The comment above it says:

                // must be a pretty broken XML file we're trying to parse here!

In my test, the file is valid XML. I’m not sure whether that means the assert shouldn’t be there or whether a different code change is the way to go.

When the assert fires, parser.getLastParseError() is “not enough input”. Sometimes, the reason the assert fires is that the existsAsFile call in File::createInputStream fails. I think there’s a hole in between the getDocumentElement(true) call and the next getDocumentElement() call in the PropertiesFile constructor. The file could disappear in between those two calls.

The loadedOK flag is correct in this case (false).

I still think FileInputStream::read should check the return value of juce_fileRead for < 0. The posix implementation of juce_fileRead can already return -1 and it seems like the currentPosition member won’t be right when it does. No need to throw an exception or communicate the error up (yet)…just to keep currentPosition correct.

Independent of that, since the posix implementation of juce_fileRead can already return -1, shouldn’t the windows implementation return -1 when ReadFile fails?


#4

I don’t quite understand the problem…?

You’ve got some crazy system where the file may randomly disappear while you’re trying to read it… When that happens, the PropertiesFile (understandably) fails to load, and correctly identifies itself as invalid. How would altering the return value of File::read make this any different?


#5

One problem is that there’s an assert firing. My understanding of asserts is that when they fire, there’s a bug in the code. Is that your understanding of asserts? Because it’s possible that a file could disappear in between the two getDocumentElement calls, and this isn’t really anything the code has control over, I think the assert should disappear.

The other problems (two of them, both independent from the first) are:

  • that that juce_fileRead doesn’t produce consistent return values across platforms. I think the windows implementation should return -1 on error just like the posix one does.
  • FileInputStream::read doesn’t check for -1 return value from juce_fileRead. I haven’t figured out the exact consequences of this, but I bet there are some bad ones.

And I can also think of a case (a new one, not one I’ve mentioned elsewhere) where isValidXML doesn’t seem like enough. If I want a blob of code whose job is to write a preference to a file, but doesn’t know the contents of the file beforehand and wants to detect errors, I think I’m stuck. If I understand correctly, the expected way to call isValidXML is like this: File a_file; PropertiesFile *props; props = new PropertiesFile(a_file,-1,PropertiesFile::storeAsXML); if (!props->isValidFile()) { // give up }

But if a_file doesn’t exist, I don’t want to give up. I want to go ahead and create the file with one preference. If I take out the call to isValidFile, I lose the ability to detect errors reading the file if it does exist. I suppose I can add an existence check: if (a_file.exists() && !props->isValidFile()) { // give up }

but then I lose if a_file exists but is an empty file or contains something that isn’t valid XML. In those cases I might want to overwrite the file with the preference, or I might want to notify some higher-up code that the file isn’t valid…but this is different than notifying that there was an error reading the file.


#6

Yes, that’s my approach to assertions now… In the past, however, I sometimes used them more loosely, to draw attention to “bad stuff happening”. This was old code, and I removed the assertion a couple of days ago, so that’s that.

Good call about juce_readFile not being consistent across platforms, though I can’t change it in the way you suggested - that would cause nasty, rare bugs for windows-only programmers who’ve written flakey code which expects the return value to always be >= 0. Plus, InputStream::read has never specified that a negative result is an error, just that it may return fewer bytes than you asked for… So I’ll need to change the posix code to always return >= 0, and leave error reporting for a future version which can use an exception to report it.

I still don’t see why you’re so bothered about finding more about the error… isValidFile() will fail if the file is unreadable or full of crap. In either case, it’s screwed, so why not just try to overwrite it - why does it matter what the reason for failure was?


#7

I see that you moved the assert from TemporaryFile to the PropertiesFile destructor. Thanks for that. There’s still one other assert that’s firing…the one on line 129 of juce_Properties.cpp.

As for the return value of juce_fileRead, I suppose you’re right, though it’s too bad. If juce_fileRead throws an exception in case of error one day that’ll resolve it more fully.

I wasn’t as concerned about the return value of FileInputStream::read as I was with the currentPosition member. Subtracting one from it in case of error could be trouble.

I’d like to find out more about errors so I can handle them better. If somehow my prefs file got corrupted, I want to know about it, to notify the user or to detect a bug in some other part of my system. Plus also, that’s something that a “retry” won’t fix. It’s different than a transient error trying to read the file.

Consider also the case of a valid file with 5 prefs in it. If I get an error reading the file and try to write a new preference to it, the original 5 prefs are gone. I’d like to avoid that. Checking a_file.exists && ~props->isValidFile handles that case, but it also triggers on a corrupted file. I might want to go ahead and blow away the corrupted file, but I don’t want to blow away the file with 5 prefs in it.


#8

I don’t understand what you must be doing to actually make all this stuff happen? Have you got multiple processes reading and writing this file simultaneously? If so, the poor old PropertiesFile was never designed to withstand that kind of treatment…


#9

I’m about to make a child class of PropertiesFile or somehow introduce an InterProcessLock to make all these problems go away. Before I do, I figured I’d create a test to expose the problems, so yes I’ve got multiple processes reading and writing the same prefs file.

In this case, I’ll have InterProcessLock to help me but when there’s no protection from InterProcessLock it makes sense to me to improve the code where possible. Plus also, I don’t think InterProcessLock is going to help me distinguish transient errors from corrupted files. It may make the corrupted files happen less often, but can’t prevent e.g. some user manually editing the file and busting it.


#10

If a user is messing with the files directly, I doubt if they’d be too surprised or expect any sympathy when something goes bang…

An interprocesslock would probably be a good option for integration into the base class, TBH.


#11

I still think learning about transient errors would be an improvement. You mentioned throwing exceptions to indicate that which sounds good to me. What can I do to help make that happen?

As far as integrating InterProcessLock into the base class, do you have in mind something like the way the Array class works? If so, I’ll see about making similar changes to PropertiesFile.


#12

Nothing yet - I don’t want to add exceptions until some other major changes are out of the way, such as the new jucer stuff. It’ll probably happen in a specially-released version, separately from other changes.

Nah… Wouldn’t want to make that class templated, it could be done quite easily just by giving it a pointer to a lock object or something like that.


#13

As I finally dive in to this I’ve quickly come to want a ScopedLock for InterProcessLock objects. One way to get there is to give InterProcessLock and CriticalSection a common base class, and to change the ScopedLock constructor to take that base class. But that might have unintended side effects. How would you approach this?

Assuming I can do something like this:

void
foo ( InterProcessLock &lock )
{
ScopedLock sl(lock);
}

I’m still not quite where I think I’d like to be. I figure the beginning of all of this is to add an argument to the PropertiesFile constructor like this:

PropertiesFile (const File& file,
                int millisecondsBeforeSaving,
                int optionFlags,
                InterProcessLock* lock = NULL);

And add a member that’s also a pointer to an InterProcessLock.

But to avoid if (lock) … everywhere, it would also be nice to add a constructor to ScopedLock to take a pointer to an InterProcessLock. This also means changing the type of the lock_ member in ScopedLock to a pointer. Not sure if the extra ifs there are unsavory enough to make this approach insufficient. I think for now I’ll create a new ScopedInterProcessLock class that takes a pointer in its constructor and leave ScopedLock alone.

Thanks for your help.

-DB


#14

I think the best approach is to add a member class or typedef like with CriticalSection::ScopedLockType and DummyCriticalSection::ScopedLockType. I’ll throw one of those in there…


#15

Thanks for adding InterProcessLock::ScopedLockType. I’m trying to use it in the PropertiesFile constructor (with new signature:PropertiesFile::PropertiesFile (const File& f, const int millisecondsBeforeSaving, const int options_, InterProcessLock* lock_) but I’m running into two snags.

  1. I think it’s right that the PropertiesFile constructor takes a pointer to an InterProcessLock object so it’s possible to use PropertiesFile without one. Do you agree? The way the InterProcessLock::ScopedLockType constructor works now I’m not sure how to create one when faced with a possibly NULL pointer. What do you think about changing the type of InterProcessLock::ScopedLockType::lock_ to be a pointer and adding a constructor that takes a pointer?

  2. What if the call to InterProcessLock::enter in InterProcessLock::ScopedLockType fails? This isn’t a problem with CriticalSection objects at least on Windows because EnterCriticalSection returns void. pthread_mutex_lock can fail, but the posix implementation of CriticalSection::enter doesn’t look at that at the moment. The InterProcessLock::Pimpl constructors (both win32 and posix implementations) can fail without throwing an exception, so InterProcesLock::enter can do the same, but users of InterProcessLock::ScopedLockType have no idea. This doesn’t seem safe.

-DB

PS It appears that the implementation of InterProcessLock is the same in juce_posix_SharedCode.h and juce_win32_Threads.cpp. Is it time for src/threads/juce_InterProcessLock.cpp?


#16

Yes, seems like a good idea.

…or you could do this kind of thing:

Good call. I’ll add an isLocked() method to match the way this is done in other scoped locks like ScopedTryLock.

The code sort of looks the same, but the pimpl classes are different, so there are actually only a couple of trivial lines of code that could actually be separated. I guess it could be templated, but that’d just clutter the headers and be overkill IMHO.


#17

Fancy. It compiles and links and works so far. Thanks.

This is a big improvement since it makes it possible to write safe code. I still think throwing an exception in the InterProcessLock::ScopedTryLock constructor is better since it makes is less likely to fail silently. As it is now, people can just leave out a call to isLocked and never know about a failure.

I figured you could leave the pimpl classes in the native files but put the top level InterProcessLock methods in juce_InterProcessLock.cpp with no template at all.


#18

I agree. But since the library doesn’t currently use any exceptions, I don’t want this to break the mould just yet. At some point in the future I’ll do a release that introduces exceptions, and will probably update the locks to use them too.

No, almost all the “common” code needs the implementation of the pimpl class in order to compile.