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

Change definition of StreamGenerator #959

Merged
merged 3 commits into from May 28, 2018

Conversation

Projects
None yet
4 participants
@jvanbruegge
Copy link
Contributor

commented May 16, 2018

Currently it is not possible to write an instance for StreamGenerator for conduit, despite the comment saying otherwise.
Furthermore you cannot use Streams that use a ReaderT IO stack.
This PR trys to solve that
DISCLAIMER: I have to idea what I'm doing

@jvanbruegge

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2018

I don't think the build failure is caused by my code

@gbaz

This comment has been minimized.

Copy link
Contributor

commented May 16, 2018

Changing the class to use a fundep makes sense, as you explained. But, I don't see what the move to a StreamGenerator parameterized over MonadUnliftIO buys you? Especially since the usage you make of it seems to all be in the case where it is specialized back to IO anyway?

@jvanbruegge

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2018

The instance I have there currently is just for easy usage with IO. My issue is because my App base monad is a ReaderT IO and I need the monadic context for my response. With MonadUnliftIO I can do that

@gbaz

This comment has been minimized.

Copy link
Contributor

commented May 16, 2018

I confess I don't understand why ReaderT IO can't be handled with the IO version directly.

@jvanbruegge

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2018

Because the return value of the function is IO (getStreamGenerator :: a -> IO ()) -> (a -> IO()) -> IO ())
If I try to define an instance for to StreamGenerator for Conduit (MonadUnliftIO implies MonadIO):

instance MonadUnliftIO m => ToStreamGenerator (ConduitT () o m ()) o where
     toStreamGenerator s = StreamGenerator $ \a b -> runConduit $ s .| op a b
        where op :: (o -> IO ()) -> (o -> IO ()) -> ConduitT o Void m ()
              op = undefined

runConduit returns m () not IO (). Thus the function is not assignable.
Maybe I am missing something here

@jvanbruegge

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2018

Yeah, UnliftIO is not needed when you let the functions return plain IO
But this actually leads to compiler errors in the code using the class:

Couldn't match type ‘m’ with ‘m1’
      ‘m’ is a rigid type variable bound by
        the instance declaration at src/MediaGoggler/Raw.hs:20:10-74
      ‘m1’ is a rigid type variable bound by
        a type expected by the context:
          forall (m1 :: * -> *).
          MonadIO m1 =>
          (o -> IO ()) -> (o -> IO ()) -> m1 ()
        at src/MediaGoggler/Raw.hs:21:27-79

This means the ToStreamGenerator class would have to pass the monad too

@gbaz

This comment has been minimized.

Copy link
Contributor

commented May 16, 2018

Right. You won't be able to write the instance in general. You can only write it for specific m. So you'll need one for ConduitT over IO, etc.

I suspect that what you've written doesn't do how you expect. The only conduits you will be able to use, because of the universal quantification, are things that work over any m that has the typeclass. This isn't going to be what you want :-)

(i.e. what you ran into above).

@gbaz

This comment has been minimized.

Copy link
Contributor

commented May 16, 2018

Oh and I see the Reader issue now. Yeah, you won't be able to pass in a conduit over ReaderT IO. There's no place to thread through the reader value to use. You'll need to convert that to a conduit over IO directly. You can do that in your ambient monad using the toIO function, I guess.

@jvanbruegge

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2018

I see, thank you. I will try that

@alpmestan

This comment has been minimized.

Copy link
Contributor

commented May 17, 2018

Regarding CI, yes the build fails with the oldest GHC that we test on but that's been the case for some time, it's not because of your patch. We will soon slide the window, getting rid of that one and adding 8.4.

@jvanbruegge jvanbruegge force-pushed the jvanbruegge:fix-stream branch 2 times, most recently from 670b4ee to 323d7e2 May 22, 2018

@jvanbruegge jvanbruegge changed the title WIP: Change definition of StreamGenerator Change definition of StreamGenerator May 22, 2018

@jvanbruegge

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2018

I squashed my commits as I was now able to write a working conduit instance that streamed a video over a normal servant StreamGet endpoint:

instance ToStreamGenerator (ConduitT () o (ResourceT IO) ()) o where
    toStreamGenerator s = StreamGenerator $ \a b -> runConduitRes $ s .| op a b
        where op :: (o -> IO ()) -> (o -> IO ()) -> ConduitT o Void (ResourceT IO) ()
              op start rest = do
                takeC 1 .| mapM_C (liftIO . start)
                mapM_C $ liftIO . rest

Code using the instance:

type FileStream = ConduitT () ByteString (ResourceT IO) ()

serveFile :: Id -> Server (StreamGet NoFraming OggVideo FileStream)
serveFile id = except404 (getFilePath id) >>= pure . Conduit.sourceFile

getFilePath :: (DB.MonadDB m) => Id -> m (Either Text FilePath)
getFilePath id = DB.getVideoFile id >>= \case
    Left e -> pure $ Left e
    Right (DBEntry _ VideoFile{ path }) -> pure . Right . toFilePath . prefixPath $ path
@jvanbruegge

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2018

And CI passes except for the oldest GHC 🎉

@jvanbruegge

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2018

Streaming files requires a basic NoFraming strategy. Should I add it to this PR? I think it is useful in general for raw data.

@alpmestan

This comment has been minimized.

Copy link
Contributor

commented May 23, 2018

@jvanbruegge Yes, please do feel free to add it. In fact, in previous discussions I came to realize that for many many common streaming scenarios, this is the "framing strategy" we would need. So this PR looks to me like the perfect excuse for adding it. :-)

@jvanbruegge

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2018

Done, though I am not entirely sure about FramingUnrender. Test do pass.

@jvanbruegge jvanbruegge reopened this May 23, 2018

@alpmestan

This comment has been minimized.

Copy link
Contributor

commented May 23, 2018

@gbaz How does this PR look to you now?

@jvanbruegge

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2018

I noticed another problem when trying to implement Range Requests over a streaming endpoint. The stream data type does not allow to specify a result code, it is always 200: https://github.com/jvanbruegge/servant/blob/db22df1236c7b02035a1ef84ba4c5ae4523864c2/servant-server/src/Servant/Server/Internal.hs#L326
Should I try to change that in this PR too, or rather open a new one?

@phadej

This comment has been minimized.

Copy link
Member

commented May 23, 2018

I managed to fix master CI, @jvanbruegge could you rebase on top of it?


I'd prefer things which can be separate in separate PRs.

@gbaz

This comment has been minimized.

Copy link
Contributor

commented May 23, 2018

This looks good to me, and what I'd expect. Glad you're doing this!

(Also, the response code stuff should def be in a different PR)

@jvanbruegge jvanbruegge force-pushed the jvanbruegge:fix-stream branch from db22df1 to a0b6d7a May 24, 2018

@jvanbruegge

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2018

Rebased

@jvanbruegge jvanbruegge reopened this May 24, 2018

@jvanbruegge

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2018

And CI is green 🎉

@phadej phadej added this to the 0.14 milestone May 28, 2018

@phadej phadej merged commit a66aa8a into haskell-servant:master May 28, 2018

1 check passed

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

@jvanbruegge jvanbruegge deleted the jvanbruegge:fix-stream branch May 28, 2018

@phadej phadej referenced this pull request Jun 25, 2018

Open

Post-841: Split Stream #995

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.