-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix memory leak in handleToFd, fixes issue #4 #222
Conversation
You probably want to replace |
-- free the spare buffers | ||
writeIORef haBuffers BufferListNil | ||
writeIORef haCharBuffer noCharBuffer | ||
writeIORef haByteBuffer noByteBuffer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this change lose any unwritten data in the write buffer? It used to be flushed, but now is just dropped?
With hClose
this happens before the call to hClose_handle_ handle_
(after which the above code is modelled):
hClose_help :: Handle__ -> IO (Handle__, Maybe SomeException)
hClose_help handle_ =
case haType handle_ of
ClosedHandle -> return (handle_,Nothing)
_ -> do mb_exc1 <- trymaybe $ flushWriteBuffer handle_ -- interruptible
-- it is important that hClose doesn't fail and
-- leave the Handle open (#3128), so we catch
-- exceptions when flushing the buffer.
(h_, mb_exc2) <- hClose_handle_ handle_
return (h_, if isJust mb_exc1 then mb_exc1 else mb_exc2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really not too sure about this change. I think it's not necessary for 2.8.0.0, since people have lived with it for over a decade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the memory leak
can be safely avoided, I'm inclined to fix anyway, but replacing flush with just dropping the buffer is not correct. Otherwise, I don't see any issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hs-viktor I'm not too familiar with this part of the code. Feel free to adjust this PR.
Btw, I tried to reproduce the original bug and the only GHC I could reproduce it with is 8.0.2 (I didn't try older ones). But with 8.0.2 is builds up pretty fast. |
Since current unix cannot be built with 8.0.2 I suggest to just close this as obsolete. |
Or we try to fix it for |
I don't see any point in |
From comment #4 (comment)
I'm not really sure this is the correct thing to do.
@rwbarton @hs-viktor