Skip to content
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

wai-app-static: use cryptonite instead of cryptohash #519

Merged
merged 3 commits into from Mar 14, 2016

Conversation

Projects
None yet
2 participants
@myfreeweb
Copy link
Contributor

commented Mar 12, 2016

cryptohash is deprecated in favor of cryptonite. Not marked as such on Hackage yet, only on GitHub. But tls uses cryptonite already, so these two packages already clash (they export the exact same API).

Also, memory instead of base64-bytestring for Base64 encoding (memory is a dependency of cryptonite).

Yes, conduit is also dropped, because cryptohash-conduit wasn't updated in a long time, and wasn't ported to cryptonite. I think it's perfectly fine to use lazy I/O here, for the sake of fewer dependencies.

@@ -120,8 +119,8 @@ webAppLookup hashFunc prefix pieces =
-- exists.
hashFile :: FilePath -> IO ByteString
hashFile fp = do
h <- Crypto.Hash.Conduit.hashFile fp
return $ B64.encode $ toBytes (h :: Digest MD5)
f <- BL.readFile fp

This comment has been minimized.

Copy link
@snoyberg

snoyberg Mar 13, 2016

Member

This has the potential to leak FDs. A better approach would be withBinaryFile, which will guarantee that the FD is closed.

This comment has been minimized.

Copy link
@myfreeweb

myfreeweb Mar 13, 2016

Author Contributor

Lazy ByteString's readFile closes the FD ("The Handle will be held open until EOF is encountered"). I've tried withBinaryFile, it results in hGetBufSome: illegal operation (handle is closed)

This comment has been minimized.

Copy link
@snoyberg

snoyberg Mar 14, 2016

Member

The correct way to do this is something like:

withBinaryFile fp ReadMode $ \h -> do
    lbs <- L.hGetContents h
    ...

The problem with readFile rears its head in the case of async exceptions. It's an uncommon case to be sure, but I try to avoid the lazy I/O functions in libraries anyway to prevent confusing behavior.

@snoyberg

This comment has been minimized.

Copy link
Member

commented Mar 13, 2016

The Travis build did not pass, can you resolve that before merge? Otherwise, the change seems good to me.

@myfreeweb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2016

Works fine now with lower dependency bounds.

@myfreeweb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2016

Oh – it was closed because the whole function was lazy.

Updated to use BangPatterns, I think it's correct now.

snoyberg added a commit that referenced this pull request Mar 14, 2016

Merge pull request #519 from myfreeweb/master
wai-app-static: use cryptonite instead of cryptohash

@snoyberg snoyberg merged commit 76e335b into yesodweb:master Mar 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@snoyberg

This comment has been minimized.

Copy link
Member

commented Mar 14, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.