URLSessionTask: implement InputStream

[albertaleksieiev]

Allows using InputStream in URLRequest as httpBodyStream. Also, implement seekInputStream in NativeProtocol.

Also added test test_dataTaskWithHttpInputStream for URLSession.

[alblue]

@swift-ci please test

[gwynne]

@swift-ci please test

[albertaleksieiev]

@swift-ci please test

[spevans]

@swift-ci please test

[spevans]

@swift-ci please test

[albertaleksieiev]

@spevans @Cellane can you please check my changes, thanks!

[Cellane]

@albertaleksieiev For me personally it looks great, and compiling a custom version of Swift with your change caused our Vapor app to finally stop randomly crashing in production, so huge thanks for that! But I have no power here 🤣

[Cellane]

LGTM, solved all my issues.

[albertaleksieiev]

@Cellane I am happy to hear it 🤗

[albertaleksieiev]

@Cellane what type of the problem do you have before? It may be a memory problem because without InputStream you need to load an entire object in memory it may cause OOM.

[millenomi]

@ianpartridge do you think this is ready to go?

[Cellane]

@albertaleksieiev I’m not quite sure, we just saw random crashes and restarts in the server log, but we failed to investigate if any particular request handler was causing it. Personally, my suspicion was on request handlers that accept images and upload them to S3, but I don’t have anything to back that claim up.

[ianpartridge]

Thanks for doing this work! I've put a few thoughts in.

[albertaleksieiev]

@ianpartridge thanks for your review, I will fix it soon!

[robipresotto]

Could we merge it? I guess a lot of people is waiting for that.

[albertaleksieiev]

@robipresotto I'll commit latest changes in a few days(most likely at weekends).

[spevans]

@albertaleksieiev This PR now has conflicts, I would suggest rebasing it on origin/master then the tests can be run.

[albertaleksieiev]

@spevans fixed! Thanks for the quick reply :)

[spevans]

@swift-ci test

[albertaleksieiev]

We can resolve it. Current libcurl only use SEEK_SET https://github.com/apple/swift-corelibs-foundation/blob/master/Foundation/URLSession/libcurl/EasyHandle.swift#L602 so we need to reset InputStream to position 0 and move forward to position. But the main problem is InputStream can't be seeked to position n if current position > n, InputStream can be moved only forward not backward. Resolution: Let’s move only forward and if curl asks us to move backward we will return CFURLSessionSeekCantSeek.

And if URLSession:task:needNewBodyStream delegate is setted we can rewind it back.

[albertaleksieiev]

Oh, I little bit was wrong here, we just need to seek forward InputStream 🙂 We assume that URLSession:task:needNewBodyStream provides new InputStream at position 0.

[albertaleksieiev]

@spevans can you please trigger a CI, thanks 🤗

[ianpartridge]

@swift-ci please test

[albertaleksieiev]

/home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux/swift-corelibs-foundation/TestFoundation/TestStream.swift:181:42: 
error: '_Error' is inaccessible due to 'internal' protection level
        } catch let error as InputStream._Error {
                                         ^
<unknown>:0: note: type declared here
/home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux/swift-corelibs-foundation/TestFoundation/TestStream.swift:157:24: 
error: 'seek' is inaccessible due to 'internal' protection level
            try stream.seek(to: pos)
                       ^
<unknown>:0: note: 'seek' declared here

It seems that NS_FOUNDATION_ALLOWS_TESTABLE_IMPORT returns false for Linux. I do "@testable import Foundation` as:

#if NS_FOUNDATION_ALLOWS_TESTABLE_IMPORT
    #if (os(Linux) || os(Android))
        @testable import Foundation
    #else
        @testable import SwiftFoundation
    #endif
#endif

I took it from here, am I doing something wrong? 😅

[albertaleksieiev]

I just get rid of NS_FOUNDATION_ALLOWS_TESTABLE_IMPORT, can someone trigger a CI? Thanks!

[spevans]

@swift-ci test

[ddunbar]

How is this PR related to #1667 ?

[albertaleksieiev]

@ddunbar It's not related to your mentioned PR at all. Here we need to seek InputStream, so I added a function seek to InputStream internal extension, and write a test for this seek function. So now our provided seekInputStream function to curl using our internal seek function.

[albertaleksieiev]

Probably I can leave seek implementation inside seekInputStream function, but it will be harder to test.

[robipresotto]

Hey Guys - We are experiencing the issue below in a swift server-side application.

Fatal error: seekInputStream(to:) is not yet implemented: file Foundation/URLSession/NativeProtocol.swift, line 264

It looks like the PR will fix the issue and wondering if someone could help me to build a swift docker image with this PR merged so we can test it properly before merging it on master.

[nerzh]

@robipresotto just reboot your app for example every 5 minutes, I am not joking, because otherwise I have a non working simple app with http protocol, I do it with all my linux projects based on swift and URLSession. Really cool, I am right ? =)

[albertaleksieiev]

Can someone please trigger a CI? Thanks 🙂

[ianpartridge]

@swift-ci test

[robipresotto]

Ship it 😅

[albertaleksieiev]

@ianpartridge What to do next? 🙃Or may someone else know?

[robipresotto]

Could we merge it?

[parkera]

@swift-ci test and merge

[albertaleksieiev]

Thanks to everyone 🙌

[ianpartridge]

Can you cherry-pick this to swift-5.0-branch?

[albertaleksieiev]

@ianpartridge for sure 👍