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

Passing resource to the subsequent conduit correctly #508

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maksar
Copy link

@maksar maksar commented Apr 25, 2024

Otherwise, temp file might be deleted by ResourceT m1

@snoyberg
Copy link
Owner

Can you provide a demonstration of the temp file being freed too early? I don't see how proper usage of the function would result in that.

@maksar
Copy link
Author

maksar commented Apr 26, 2024

:set -XOverloadedStrings

import Conduit as C
import Data.Conduit.Binary as CB

C.runConduitRes (C.yield "contents" C..| CB.sinkCacheLength) >>= \(_, c) -> C.runConduitRes (c C..| C.sinkList)

Gives:

*** Exception: /tmp/conduit59986-2.cache: openBinaryFile: does not exist (No such file or directory)

@snoyberg
Copy link
Owner

That program is behaving as expected. The idea with ResourceT is that, whenever runResourceT is called, all resources are destroyed. runConduitRes implicitly calls runResourceT, so you end up closing the temp file in the first runConduitRes call. Instead, you can wrap the whole thing in runResourceT and then use runConduit instead of runConduitRes.

I believe the change in this PR will result in not performing cleanups in cases of non-termination, such as an exception or not consuming all input.

@maksar
Copy link
Author

maksar commented Apr 26, 2024

I agree with "non-cleanup in case of non-consuming" argument, so we end up creating own function.

Our use-case is streaming from S3, which isn't very reliable for long-running slow consumption steams. So we decided to proxy through temp file. It is beneficial for us to terminate all resources related to S3 streaming quickly and then slowly stream from temp file.

Function signature kind of suggested that user has a freedom of terminating first resourceT before terminating second – m1 and m2 are distinct. Maybe it worth at least unifying:

(MonadResource m) => ConduitT ByteString o m (ConduitT i ByteString m ())

Which would protect from calling runResourceT twice on accident:

# this would become impossible
runResourceT (runConduitRes (yield "contents" .| sinkCacheLength) >>= \(_, c) -> runConduit (c .| sinkList))
# only that will compile
runResourceT (runConduit (yield "contents" .| sinkCacheLength) >>= \(_, c) -> runConduit (c .| sinkList))
# well, this would still compile, but ResourceT (ResourceT m) type of c would indicate the probable coding mistake
runResourceT (runConduitRes (yield "contents" .| sinkCacheLength) >>= \(_, c) -> runConduitRes (c .| sinkList))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants