Fix for 13839: Crash on opening file with error "sharing violation".

[nethip]

Reverting the file opening to the older format as the older approach seems better than the newer approach. Also this will resolve the crash on opening a file which is being exclusively held by another process.

Fixes adobe/brackets#13839

@saurabh95 Will you be able to review this?

[swmitra]

LGTM. That invalid handle check is what makes the class approach look odd.

[saurabh95]

Apologies for the late response. Just saw the PR
I changed the old approach in order to make sure that if there is some error in the detection of encoding or some other error in reading then the file handle should automatically get closed. I had a discussion with @abhijitapte regarding the same and that's why we went with RAII design pattern, so in case of error scenarios the destructor will be called and file handle will be closed.

I am not sure how this PR fixes the issue discussed.
@nethip Could you please elaborate on it?

[nethip]

@saurabh95 No issues Saurabh! Thanks for looking at the PR!

There are fundamental problems with the class that was introduced which had to take care of getting a valid file handle.

1. An exception was getting thrown in the constructor which is a bad idea.
2. The exception thrown was not caught anywhere. This is the main reason for the crash as the exception was leading to UnHandled Exception case.
3. Though the class seem to be managing the file handle, it does not own the file handle, anymore. Someone can call operator HANDLE and for any reason once the class goes out of scope the file handle is getting closed. There is no guarantee if the file HANDLE is still in use or not. Since there is no reference counting mechanism going on here, class members life time has to be managed by the class itself.

I checked for any other exception scenarios and figured out that most of the code is anyways wrapped inside try..catch, which was the reason why I brought back the older code.

[nethip]

Thanks @swmitra !

[saurabh95]

@nethip Yeah it makes sense when we have try..catch everywhere then we remove the RAII for file handle.
But I am not able to understand how both the approaches are different
In both the approaches, we are creating the file handle. In the first approach, we just return with an error in case we are not able to get handle and in second approach(RAII) we raise an exception which is not caught. And in case of no failures the scope of the class instance (in RAII approach) and scope of file handle (in the current approach) is limited to the function only and in both the cases CloseHandle would be called at the same time.

So ideally, if we wrap the ReadFileHandle readFileHandle(filename); in try..catch, then both the approaches are essentially same.

So, I am not able to get how this actually solves the problem or not. Also, it seems that @devdmore faced this issue recently while this code has been there for a while now.

Can we do a pre-release and share it with @devdmore so that we can be sure that the issue is resolved?

[nethip]

@saurabh95 We are able to repro this bug 100%, and with the older approach the crash seems to be resolved.

We already have a pre-release planned in the coming weeks. So we could surely invite users to try out the build and see if they face any issues.

Regarding the actual fix: If the approaches are same, then I would recommend sticking to the earlier format especially because of the problems I have sited in the comment above and also because of the testing impact. If older code, is tried and tested and is working for a long time, I would rather go with the older approach than the newer one, unless you there is a strong reason for doing it.

About error handling, we are already handling the error cases by returning, if the handle is invalid. CreateFile is going to return an error code, in case of errors rather than exceptions.

In brackets-shell, error codes are preferred to exceptions.

So if you still feel that having a new class is the way to go, please feel free to raise a PR with your recommended changes. We would review it. Please follow the below guidelines.

1. Avoid throwing exceptions in constructor.
2. Wrap the entire ReadFile code inside your ReadFileHandler class and add a method like ReadFile. Otherwise it looks very inconsistent as a part resides in a class and the rest inside a method.
3. Initialize the class members by having a constructor.
4. If you are throwing exceptions inside your class, make sure you have the appropriate catch handler at all the places where the method is called.

[saurabh95]

@nethip, I totally agree with you, the current approach (non-RAII) is better, and there were definitely some mistakes(which you pointed out) in the RAII approach. And also the error handling in non-RAII approach is better as you mentioned.