attempt at fix dungbeetle alert

[buzinsky]

Right now untested. The errors seem to suggest that this is totally coming from the getsize() call which only happens here. When it acts on a file that does not exist, it throws this error.

This should be fixed by this change, as isfile(x) is not logically the same as not isdir(x). While they are the same statements if I put x = filename or directory name (true, false respectively), when the file does not exist, the statements are not the same. We now check explicitly that this is a real file

Resolves #212

[wcpettus]

I think there may still be an unhandled race condition, albeit we've decreased the cross-section for this happening -

• The del_dir method is called in two locations after creating a listdir on a directory:
  https://github.com/project8/dragonfly/blob/master/dragonfly/implementations/dungbeetle.py#L79
  https://github.com/project8/dragonfly/blob/master/dragonfly/implementations/dungbeetle.py#L56
  ^ the second is more important because the first only handles the listed root directories to watch

• Then a path is constructed and no_sub_dir condition is checked, then del_dir is called

• Switching from isdir to isfile will fix the conditions where the file is purged while the above are happening.

• After that check, the file can still be deleted during the .nfs check or during the getsize check (wherein the creation_time check will fail).

Should we do something more generic? If we wrap everything starting here:
https://github.com/project8/dragonfly/blob/master/dragonfly/implementations/dungbeetle.py#L51
in a try/except OSError block, we could catch every possible race condition. We could even give a different warning message depending on the result of os.path.exists(path), to check if the file has been removed.

Is this overkill, have we seen any other edge cases?

[buzinsky]

I agree that all I did was lower the cross section of alerts (I probably should have write this out to save you some time analyzing), and, in principle, this sequence of events causing an error you describe could happen.

My reasoning for sufficiency was that when I do go through slack, the getsize has been the source of all the errors, and none from getctime, so the sequence of events hasn't happened yet.

I think using some sort of wrapper is the better way to do this. I'd rather put this into a separate issue/ future upgrade. Otherwise nothing we do can really be 100% safe. Eg. you could do your isfile(), then dirac deletes it, then we call getsize(), which throws this same error. Very unlikely, and very very tough to completely protect against.

If you see a good reason why you think my change would increase errors from getctime, let me know. I don't think so, but we could add a quick isfile, before doing some wrapping in the longer term

Edit: Hmmm now I want to think about your proposed solution some more.

[buzinsky]

@wcpettus I am confused by this statement:
Should we do something more generic? If we wrap everything starting here:
What do you mean "everything" in this statement? Are you suggesting we just need to wrap the next two lines (51/52) in a try? Or do you want to put the rest of del_dir in a try block? I ask because I didn't think the scoping would work if you just put line 51-end of function in a try block

I do think what you suggest is easy and thorough, however

[wcpettus]

Yes, option 1 that you suggest (we just need to wrap the next two lines (51/52) in a try?)

I think it has the further advantage that the except OSError that you put there could supersede the try/except of lines 40/43. I think it could be extended to have an if os.path.exists(path) then you post a warning, but if the file no longer exists than we understand how we got there and don't even need a warning to go out.