kotlinlang #kotlin-fuel

There was also this issue: https://github.com/kittinunf/Fuel/issues/482. Some obscure error on android 4, which, I just checked, can't actually be fixed by us and caused by the type definition.

kittinunf

10/25/2018, 4:14 PM

approved 🙂

💯 2

sbyrne

10/25/2018, 7:46 PM

I switched to the long setFixedLengthStreamingMode, and then I get IOExceptions out of HttpURLConnection later on.

sbyrne

10/25/2018, 7:47 PM

why is bodyCallback called twice? (in HttpClient.setBodyIfDoOutput())

Derk-Jan Karrenbeld

10/25/2018, 8:28 PM

That's an issue with the implementation which I am fixing in my PR. The idea here is that they wanted to have it get the contentlength using the same function as the body but not all streams can rewind so that doesn't work. Untill that is fixed, What you should do in your thing is check if output stream is null. If so, don't actually consume your input. That is the workaround for now

Derk-Jan Karrenbeld

10/25/2018, 8:28 PM

(this is how I understood it the first time I encountered this myself)

Derk-Jan Karrenbeld

10/25/2018, 8:28 PM

In the new implementation the stream is read only once.

Derk-Jan Karrenbeld

10/25/2018, 8:33 PM

You can see this in uploadrequesttask

Derk-Jan Karrenbeld

10/26/2018, 4:33 AM

@sbyrne the PR is up. not tested well and not quite done but should solve these issues.

sbyrne

10/26/2018, 3:16 PM

I had a hard deadline to have something working, so I switched to Apache Http Client. I am not sure when I'll have time to test Fuel again.

Derk-Jan Karrenbeld

10/26/2018, 3:18 PM

No problem! Thanks for your input tho

Derk-Jan Karrenbeld

10/26/2018, 4:20 PM

Wooooo fixed the high memory issues, and the double stream consumptions :3

sbyrne

10/26/2018, 4:54 PM

woo! Hopefully I'll have some time to try it out next month.

❤️ 1

Derk-Jan Karrenbeld

10/27/2018, 7:33 PM

https://github.com/kittinunf/Fuel/pull/481 headers are ready

Derk-Jan Karrenbeld

10/28/2018, 4:16 PM

@Mark Gilchrist is this what you wanted in terms of testability? https://github.com/kittinunf/Fuel/commit/d616b4c447dd43784dd6ecbf4fdc1e72d483c1d1 It allows you to inspect the reflected mock, so we can get rid of all those "containsString" and actually see if it's in the correct place.

Nikky

10/28/2018, 4:18 PM

nice

💯 1

Mark Gilchrist

10/28/2018, 4:23 PM

Will check it out. I sure it will be everything and more!

Mark Gilchrist

10/28/2018, 4:25 PM

That is exactly it, does this test pass?

Derk-Jan Karrenbeld

10/28/2018, 4:47 PM

yes

Derk-Jan Karrenbeld

10/28/2018, 4:48 PM

We can gradually rewrite tests using that

Derk-Jan Karrenbeld

10/28/2018, 4:48 PM

(once it's merged :P)

Derk-Jan Karrenbeld

10/28/2018, 4:49 PM

(but there is an open issue which might be related. Facebook basically saying nope. Don't know why, but will test later with a local server, see what actually comes in)

Mark Gilchrist

10/28/2018, 6:16 PM

Ok, I added some comments to you Prs some great work there mate

Derk-Jan Karrenbeld

10/28/2018, 6:25 PM

Sweet! Thanks

Derk-Jan Karrenbeld

10/29/2018, 1:12 AM

Thank you for the review @Mark Gilchrist! I'll make that one change (

O(n)

O(1)

) and then I hope it's ready to merge (header normalisation thing)

Derk-Jan Karrenbeld

10/29/2018, 1:26 AM

Done!

Mark Gilchrist

10/29/2018, 7:07 AM

looking very nice @Derk-Jan Karrenbeld

Derk-Jan Karrenbeld

10/29/2018, 4:34 PM

Next up: https://github.com/kittinunf/Fuel/pull/485

Derk-Jan Karrenbeld

10/29/2018, 4:36 PM

I'll write documentation changes once you all look at it and potential changes are made 😛

Nikky

10/29/2018, 4:37 PM

kotlin 1.3 is out , whats the plan ? finish all existing PRs and then bump ?