<div class="gmail_quote">On Thu Nov 13 2014 at 8:58:12 AM Yuras Shumovich <<a href="mailto:shumovichy@gmail.com" target="_blank">shumovichy@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Thu, 2014-11-13 at 00:43 +0200, Eyal Lotem wrote:<br>
> ><br>
> > -- | database that uses two files<br>
> > data DB = DB Handle Handle<br>
> ><br>
> > closeDB :: DB -> IO ()<br>
> > closeDB (DB h1 h2) = hClose h1 >> hClose h2<br>
> ><br>
> > The cleanup action "closeDB" above is buggy because the first hClose can<br>
> > be interrupted. In that case the first handle will be closed, but the<br>
> > second will leak. Note: "closeDB" is not atomic -- it consists from two<br>
> > interruptible different actions. The same with hClose itself -- if can<br>
> > be interrupted somewhere in the middle, but it is able to handle that.<br>
> ><br>
> > The correct cleanup probably should look like the next:<br>
> ><br>
> > closeDB (DB h1 h2) = hClose h1 `finally` hClose h2<br>
> ><br>
> > Note: the initial version is buggy with respect to both async and sync<br>
> > exceptions, and uninterruptibleMask will fix it only with respect to<br>
> > async exceptions.<br>
> ><br>
> > The second version is (I hope) exception-safe -- it handle both async<br>
> > and sync exceptions. That is important point -- if you need<br>
> > uninterruptibleMask, then probably you have issue with sync exceptions<br>
> > too. Lets fix the original issue and make code exception safe instead of<br>
> > hiding it behind uninterruptibleMask.<br>
> ><br>
><br>
> Your second version is not exception-safe: async exceptions would just<br>
> cease the first close, leak the handle and continue to the second close to<br>
> potentially block again and either leak or close the second handle. This is<br>
> not reasonable behavior for cleanup. If you wrap it all with<br>
> uninterruptibleMask then it becomes as correct a cleanup as it can be.<br>
<br>
You are wrong, hClose closes the handle in case of any exception, so<br>
there is no leak here. I already described that and pointed to source<br>
code. Probably my arguments are weak, but nobody even tried to argue the<br>
opposite. The relevant part:<br></blockquote><div><br></div></div><div class="gmail_quote"><div>People have been arguing the opposite. hClose is not guaranteed to close the handle in case an exception arises. Here's a demonstration program.</div><div><br></div><div><div>> module Main where</div><div>> </div><div>> import System.IO</div><div>> import Network.BSD</div><div>> import Network.Socket</div><div>> import Control.Concurrent</div><div>> import Control.Exception</div><div>> import Control.Applicative</div><div>> import Control.Monad</div><div>></div><div><div>> main = do</div><div>> sock <- socket AF_INET Stream 0</div><div>> addr <- SockAddrInet 7777 <$> lookupHost "localhost"</div><div>> setSocketOption sock ReuseAddr 1</div><div>> bindSocket sock addr</div><div>> listen sock 6</div><div>> forkIO $ do</div><div>> tid <- myThreadId</div><div><div>> forkIO $ do</div><div>> sleep 10</div><div>> print "killing"</div><div>> forkIO $ killThread tid >> print "killed it"</div><div>> return ()</div></div><div>> bracket (opener sock) closer $ \h -> do</div><div>> forkIO $ listener h</div><div>> sleep 2</div><div>> print "sleeping"</div><div>> sleep 120</div><div>> </div><div>> listener h = forever $ do</div><div>> inp <- hGetLine h</div><div>> print inp</div><div>> </div><div>> opener sock = do</div><div>> (s',addr) <- accept sock</div><div>> print $ "Got connection from: " ++ show addr</div><div>> socketToHandle s' ReadWriteMode</div><div>> </div><div>> closer h = (hClose h `finally` print "closed")</div></div><div>> <br></div><div>> sleep :: Double -> IO ()</div><div>> sleep = threadDelay . round . (* 1e6)</div><div>> </div><div>> lookupHost n = head . hostAddresses <$> getHostByName n</div></div><div><br></div><div>I compiled this with ghc-7.8.3 -O -threaded and ran it, then connected to localhost:7777 via nc, waited until "closed" was printed, then sent some data. This was the result:</div><div><br></div><div><div>jwlato@burial:~/explorations$ ./HClose </div><div>"sleeping"</div><div>"Got connection from: <a href="http://127.0.0.1:45949" target="_blank">127.0.0.1:45949</a>"</div><div>"killing"</div><div>"killed it"</div><div>"closed"</div><div>"foo"</div><div>"bar"</div><div>"baz"</div><div>HClose: <socket: 11>: hGetLine: end of file</div></div><div><br></div><div>Note that "closed" was printed, so we should assume that hClose had a chance to run as well. The handle clearly was not closed (I confirmed this with lsof as well).</div><div><br></div><div>This result is consistent with the async exception arising while hClose is blocked internally on an MVar.</div><div><br></div><div>If you instead wrap "closer" in uninterruptibleMask_, the result is quite different:</div><div><br></div><div><div>jwlato@burial:~/explorations$ ./HClose </div><div>"sleeping"</div><div>"Got connection from: <a href="http://127.0.0.1:46302" target="_blank">127.0.0.1:46302</a>"</div><div>"killing"</div><div>"foo"</div><div>"closed"</div><div>HClose: <socket: 11>: hGetLine: illegal operation (handle is closed)</div><div>"killed it</div></div><div><br></div><div>Note that this time, killThread didn't return immediately, because the async exception had not been delivered. As soon as I sent a line of data over the socket, hGetLine finished and unblocked hClose, which then closed the handle.. The next loop of hGetLine then failed, and concurrently killThread returned.</div><div><br></div><div>You might argue that hClose should use uninterruptibleMask internally (which is the only way to fix the issue). Possibly so. However, this is a really pervasive problem, which is why it makes some sense to place the mask in bracket and fix every handler properly.</div><div><br></div><div>At some point in this thread a person (you?) has argued that this isn't a problem in practice. I disagree. It actually seems to be fairly common in certain types of network programming.</div><div><br></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> > It is already implemented in such the way. Let me explain.<br>
> > > There are two sources of possible interruptions in hClose:<br>
> > > a) takeMVar<br>
> > > b) flushing internal buffer<br>
> > ><br>
> > > a) is not an issue in practice -- it will not be interrupted<br>
unless<br>
> > > someone already uses the Handle (if it is the case, then you<br>
probably<br>
> > > has bigger issue -- you may use already closed handle.) But it<br>
probably<br>
> > > should be more careful and use uninterruptibleMask here... I don't<br>
have<br>
> > > strong opinion.<br></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> > > b) is handled correctly, see<br>
> > ><br>
> ><br>
<a href="https://github.com/ghc/ghc/blob/805ee118b823f271dfd8036d35b15eb3454a95ad/libraries/base/GHC/IO/Handle/Internals.hs#L734" target="_blank">https://github.com/ghc/ghc/<u></u>blo<u></u>b/<u></u>805ee118b823f271dfd8036d35b1<u></u>5e<u></u>b3454a95ad/libraries/base/<u></u>GHC/<u></u>IO/Handle/Internals.hs#<u></u>L734</a><br>
> > > Basically it catches all exceptions (including async,) closes the<br>
handle<br>
> > > and rethrows the exception.<br>
><br>
<br>
"closes the handle" here means that "close" method of underlying<br>
IODevice is called. And now it is IODevice's author responsibility to<br>
handle exceptions correctly.<br></blockquote><div><br></div><div>except that isn't guaranteed, as my program demonstrates.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
><br>
> Sync exceptions when closing your DB handles leave it in an undefined state<br>
> (or at least, that's the underlying behavior of POSIX close). At that<br>
> point, the handles cannot be re-closed (since they may have been reused in<br>
> a different context).<br>
><br>
> So sync exceptions in hClose mean the program is incorrect, and the only<br>
> recourse is to prevent the sync exceptions in the first place. Fortunately,<br>
> these FDs are likely guaranteed to be valid so sync exceptions are<br>
> virtually ruled out.<br>
><br>
> This is a general pattern with cleanups: a cleanup already has the<br>
> allocated resource at hand, which almost always rules out sync exceptions.<br>
> Also, exceptions during an error-induced cleanup cause dangerous<br>
> error-silencing anyway since we cannot handle an exception within an<br>
> exception.<br>
<br>
So you have to inspect all the code, directly or indirectly used by<br>
cleanup action, to ensure it doesn't throw sync exception (just to find<br>
that it is not the case -- a lot of cleanup actions can throw sync<br>
exceptions in some, probably rare, cases.) Someone argued, that was<br>
exactly the issue the proposal was trying to solve.<br></blockquote><div><br></div><div>Sync exceptions have nothing to do with the proposal. The proposal itself certainly doesn't argue this.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
><br>
> In my opinion you're incorrectly equating sync and async exceptions. The<br>
> former can only be avoided by satisfying preconditions which you must do in<br>
> cleanups. The latter can only be avoided by uninterruptibleMask, which you<br>
> must also do in cleanups. The combination of the two is the only way to<br>
> make exception-safe cleanups that:<br>
<br>
I'm not equating them, I'm arguing for exception safe code. </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Don't lie yourself, hClose can throw sync exception and it *will* throw<br>
it sooner or later. If you are not prepared for that, you'll get<br>
mysterious bug. But if you are prepared, then just don't need<br>
uninterruptibleMask in bracket.<br></blockquote><div><br></div><div>Again, this has nothing to do with hClose throwing sync exceptions. It does have to do with handlers that perform blocking operations and don't use uninterruptibleMask.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
><br>
> A) Do not break invariants if async exception is sent during cleanup<br>
> B) Do not cause an exception within an exception (e.g: during bracket,<br>
> onException or finally) where at least one exception must be lost, which is<br>
> yet another bug which was overlooked in this discussion<br>
<br>
It is not overlooked (I even posted link to discussion of this issue in<br>
the my fist reply to the thread.) But it is simply not relevant.<br>
<br>
<br>
> If hClose throws a sync exception there's *nothing* that can be done to<br>
> make the code not leak the resource.<br>
><br>
> However:<br>
><br>
> bracket openFile hClose -- is correct with uninterruptibleMask and<br>
> incorrect with mask. The potential for a sync exception in hClose here is<br>
> irrelevant to the correctness that can be attained.<br>
><br>
> So it *does* in fact make writing exception-safe code much much easier.<br>
<br>
Could you please point me to line in source code where hClose can throw<br>
exception without calling IODevice.close? Where it can be interrupted by<br>
async exception? And if you find such places, then why should not it be<br>
fixed?<br>
<br>
<br>
If you find that uninterruptibleMask makes your life easer, then go<br>
ahead and use it. Sometimes it is even necessary to make code exception<br>
safe. But it is bad idea to use it in bracket from base because it<br>
actually only hides bug, not fixes them. As a result more bugs will<br>
remain unnoticed and not fixed for longer period.<br>
<br>
Thanks,<br>
Yuras<br>
<br>
<br>
______________________________<u></u><u></u>_________________<br>
Libraries mailing list<br>
<a href="mailto:Libraries@haskell.org" target="_blank">Libraries@haskell.org</a><br>
<a href="http://www.haskell.org/mailman/listinfo/libraries" target="_blank">http://www.haskell.org/<u></u>mailman<u></u>/listinfo/libraries</a><br>
</blockquote></div>