Skip to content

Streaming request body for servant-client-core#913

Merged
jkarni merged 6 commits intomasterfrom
jkarni/expose-more-request-constructors
Mar 23, 2018
Merged

Streaming request body for servant-client-core#913
jkarni merged 6 commits intomasterfrom
jkarni/expose-more-request-constructors

Conversation

@jkarni
Copy link
Member

@jkarni jkarni commented Feb 27, 2018

Fixes #886.

I also added tests to check that Stream ran in constant memory while I was at it.

@jkarni
Copy link
Member Author

jkarni commented Mar 6, 2018

Tests are failing on 7.8.4 for reasons that don't have anything to do with the PR. I tried reproducing it locally but can't. @phadej any ideas?

@jkarni jkarni force-pushed the jkarni/expose-more-request-constructors branch from bb49f90 to e3158d7 Compare March 8, 2018 17:38
@jkarni
Copy link
Member Author

jkarni commented Mar 11, 2018

@phadej @alpmestan can I merge?

@phadej
Copy link
Contributor

phadej commented Mar 11, 2018 via email

@phadej
Copy link
Contributor

phadej commented Mar 12, 2018

I took a quick glance and I'm not particularly fond of replicating http-client API in the servant-client-core. It feels like a step back. I'Il think a little harder tomorrow

@jkarni
Copy link
Member Author

jkarni commented Mar 13, 2018

I took a quick glance and I'm not particularly fond of replicating http-client API in the servant-client-core

I thought that was what we had come up with over IRC. But if there are nicer alternatives I'm all for them. The only constructor in RequestBody that didn't seem necessary/logical was RequestBodyIO (but funnily I ended up using it in servant-streaming, and now am in favor of it).

@jkarni
Copy link
Member Author

jkarni commented Mar 14, 2018

Any updates?

where
lotsGenerator f r = do
f ""
withFile "/dev/urandom" ReadMode $
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use random / tf-random etc. and not drain the (somewhat slow) secure random device?

Copy link
Member Author

@jkarni jkarni Mar 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried random and bytestring-random, but they used a lot more CPU and failed (without clear error messages) on CI.

I can remove this test entirely, though

Copy link
Contributor

@phadej phadej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one unrelated to streaming change request, otherwise this is ok. We can change the servant-client-core RequestBody again when we are wiser.

Also, does it work with servant-client-ghcjs, if there is an easy way to check, it would be great.

EDIT s/Request/RequestBody/

@jkarni
Copy link
Member Author

jkarni commented Mar 18, 2018

Good call on testing GHCJS. I'm trying to do that but keep running into problems. Will update here when it works.

@jkarni jkarni force-pushed the jkarni/expose-more-request-constructors branch from 672ff4a to ba33b61 Compare March 19, 2018 15:26
jkarni added 4 commits March 19, 2018 16:26
    Mimicking http-client's RequestBody.
    Streaming is not actually performed - instead the whole object is
    held in memory.
@jkarni jkarni force-pushed the jkarni/expose-more-request-constructors branch from ba33b61 to 41ca5ad Compare March 19, 2018 15:27
jkarni added 2 commits March 19, 2018 18:10
    Using random packages mysteriously fail on CI, and also uses a lot
    more CPU.
@jkarni jkarni force-pushed the jkarni/expose-more-request-constructors branch from 41ca5ad to 7c901dc Compare March 19, 2018 17:23
@domenkozar
Copy link
Contributor

@jkarni how can I help get this finalized?

@jkarni jkarni merged commit 3750f22 into master Mar 23, 2018
@jkarni jkarni deleted the jkarni/expose-more-request-constructors branch March 23, 2018 17:30
@jkarni
Copy link
Member Author

jkarni commented Mar 23, 2018

@domenkozar finalized :)

Well, at least, modulo releasing it.

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.

3 participants