Fix readFile leaking file descriptors in the presence of asynchronous exceptions

[prednaz]

readFile leaks file descriptors in the presence of asynchronous exceptions. I confirmed that with this test. I would have applied the nice fix for Data.Text.IO.readFile to Data.Text.Lazy.IO.readFile too but it does not get along well with lazy IO because the file handle will immediately be closed before reading is done.

[prednaz]

same issue with base package

[Yuras]

Will adding performMajorGC before getLine in the test fix the issue? I didn't try, but I think it should help.

But we can do better job closing handle asap, you are right here. Though I don't think the issue is specific to asynchronous exceptions, so your fix (at least for lazy case) is incomplete. Consider the following case: openFile succeeds, but then hGetContents fails before turning handle into semi-closed state. E.g. chooseGoodBuffering inside hGetContents might fail with synchronous exception. In that case handle won't be closed.

So consider something like braketOnError (openFile name ReadMode) hClose hGetContents. I think it should work in case of both asynchronous and synchronous exceptions, and also lazy and strict readFile. The same probably applies to readFile in base.

(And thank you for raising the issue!)

[treeowl]

I think the biggest smell is in openFile itself. If I'm not very much mistaken, that can be interrupted by an asynchronous exception between opening the FD and creating a finalizer to close it. In that case, the FD is left open until the whole process dies.

[treeowl]

@Yuras, I don't think any bracketing can work for the lazy readFile. For that, the key is to make sure that the handle (and its associated finalizer) is actually created if the file is open. For the strict one, bracketing definitely looks like the right solution.

[Yuras]

@treeowl I checked openFile and I agree that it's not exception safe. Though, again, I don't see how it's specific to asynchronous exceptions, unless there is a good reason to believe that getLocaleEncoding can never throw synchronous exception. (I'd use bracketOnError FD.openFile IODevice.close $ ... here too)

But I'm not sure I understand why bracketOnError won't work here. It will guarantee that either handler is turned into semi-closed or it's closed immediately. Note the difference between bracket and bracketOnError.

[treeowl]

I see no benefit to promptly turning the handle semi-closed. Who cares?

[Yuras]

I see no benefit to promptly turning the handle semi-closed. Who cares?

It's a different question :) If nobody cares, then we only need to fix openFile.

[treeowl]

I don't think anyone can care. For semi-closed to matter, someone has to have the actual handle. But readFile doesn't expose the handle, so no one else can try to operate on the handle.

[Yuras]

@treeowl Now I think you are misreading my comment... (sorry if it's not the case). It's about promptly closing the handle, not about turning it into semi-closed.
When exception (sync or async) is thrown after openFile returns, but before hGetContents turns handle into semi-closed, then we can either close the handle immediately (that's what the PR is trying to do) or rely on finalizer to close in on the next GC (what we have now, modulo the issue in openFile itself). I'm not sure whether the difference is huge, but it might be important in some cases. Essentially it's a question of whether you'll unexpectedly reach the limit on number of open FDs.

[prednaz]

I agree, that the change to Data.Text.Lazy.IO.readFile is quite pointless. I did not know about the garbage collector triggering finalizers.

[treeowl]

@Yuras, everyone wants the handle closed promptly, but I don't think you can actually do that in a meaningful way in the lazy case. Yes, you can deal with an exception that happens before hGetContents really gets going, but there's nothing you can do about prompt closure if the exception happens later, while the contents are being consumed. Do you have some application in mind where being prompt in that one special case is particularly important?

[Yuras]

@treeowl Yes, that's exactly what I meant: one could handle exceptions between the end of openFile and the end of hCloseContents (where handle is converted to semi-closed). I already wrote that I have no idea whether it's important enough to care, and I'm not trying to convince anyone to actually handle them. I though @prednaz cared about this case (note that I wrote the first comment assuming openFile is correct), but looks like he didn't, so I guess there is no point in further discussion.

[treeowl]

Sorry, wasn't trying to insult you or anything, and I definitely did miss that subtlety. This stuff is all *terribly* subtle.

…

On Wed, Dec 23, 2020, 5:58 PM Yuras ***@***.***> wrote: @treeowl <https://github.com/treeowl> Yes, that's exactly what I meant: one could handle exceptions between the end of openFile and the end of hCloseContents (where handle is converted to semi-closed). I already wrote that I have no idea whether it's important enough to care, and I'm not trying to convince anyone to actually handle them. I though @prednaz <https://github.com/prednaz> cared about this case (note that I wrote the first comment assuming openFile is correct), but looks like he didn't, so I guess there is no point in further discussion. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#310 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOOF7JJ7FB6EEDFDNKP5X3SWJY2ZANCNFSM4VHNSOIQ> .

[treeowl]

Still, I don't think there's enough time between finalizer creation and return from hGetContents to be practically significant. We have to give up on promptness *somewhere*, and return from openFile seems as good a place as any.

…

On Wed, Dec 23, 2020, 6:01 PM David Feuer ***@***.***> wrote: Sorry, wasn't trying to insult you or anything, and I definitely did miss that subtlety. This stuff is all *terribly* subtle. On Wed, Dec 23, 2020, 5:58 PM Yuras ***@***.***> wrote: > @treeowl <https://github.com/treeowl> Yes, that's exactly what I meant: > one could handle exceptions between the end of openFile and the end of > hCloseContents (where handle is converted to semi-closed). I already > wrote that I have no idea whether it's important enough to care, and I'm > not trying to convince anyone to actually handle them. I though @prednaz > <https://github.com/prednaz> cared about this case (note that I wrote > the first comment assuming openFile is correct), but looks like he > didn't, so I guess there is no point in further discussion. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#310 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAOOF7JJ7FB6EEDFDNKP5X3SWJY2ZANCNFSM4VHNSOIQ> > . >

[Yuras]

Sorry, wasn't trying to insult you or anything

Oh, sorry, it's my fault, I didn't mean anything like that. I was just trying to wrap up the conversation. I'm not a native speaker, and I fail to correctly express myself too often. No offense taken.