Enforce exclusive memory access to delegate in ClientRequest

[nethraravindran]

This is to ensure Swift 4's Memory Safety model - Enforce Exclusive Access to Memory
(Ref: https://github.com/apple/swift-evolution/blob/master/proposals/0176-enforce-exclusive-access-to-memory.md)

Motivation and Context

Encountered the following crash with message Simultaneous accesses to 0x7f16a000a938, but modification requires exclusive access on delegate?.prepareForRedirect() in ClientRequest

Simultaneous accesses to 0x7f16a000a938, but modification requires exclusive access.
Previous access (a modification) started at Kitura-netPackageTests.xctest`<unavailable> + 254605 (0x562b9e62028d).
Current access (a read) started at:
0    libswiftCore.so                    0x00007f16e23bb840 swift_beginAccess + 510
1    Kitura-netPackageTests.xctest      0x0000562b9e620da1 <unavailable> + 257441
2    Kitura-netPackageTests.xctest      0x0000562b9e622ee1 <unavailable> + 265953
3    Kitura-netPackageTests.xctest      0x0000562b9e621006 <unavailable> + 258054
4    Kitura-netPackageTests.xctest      0x0000562b9e622f2b <unavailable> + 266027
5    libswiftCore.so                    0x00007f16e223f490 withUnsafePointer<A, B>(to:_:) + 12
6    libswiftCore.so                    0x00007f16e23a40c0 withUnsafeMutablePointer<A, B>(to:_:) + 9
7    Kitura-netPackageTests.xctest      0x0000562b9e62036b <unavailable> + 254827
8    Kitura-netPackageTests.xctest      0x0000562b9e61ad9c <unavailable> + 232860
9    Kitura-netPackageTests.xctest      0x0000562b9e6bea0d <unavailable> + 903693
10   Kitura-netPackageTests.xctest      0x0000562b9e693bb2 <unavailable> + 727986
11   Kitura-netPackageTests.xctest      0x0000562b9e69bc1c <unavailable> + 760860
12   Kitura-netPackageTests.xctest      0x0000562b9e685fef <unavailable> + 671727
13   Kitura-netPackageTests.xctest      0x0000562b9e69bc71 <unavailable> + 760945
14   Kitura-netPackageTests.xctest      0x0000562b9e6bcce8 <unavailable> + 896232
15   Kitura-netPackageTests.xctest      0x0000562b9e6bd021 <unavailable> + 897057
16   Kitura-netPackageTests.xctest      0x0000562b9e6bcd35 <unavailable> + 896309
17   Kitura-netPackageTests.xctest      0x0000562b9e6bcf65 <unavailable> + 896869
18   Kitura-netPackageTests.xctest      0x0000562b9e6354ed <unavailable> + 341229
19   libdispatch.so                     0x00007f16e3003337 <unavailable> + 287543
20   libdispatch.so                     0x00007f16e300f82a <unavailable> + 337962
21   libdispatch.so                     0x00007f16e300ffdd <unavailable> + 339933
22   libdispatch.so                     0x00007f16e301268a <unavailable> + 349834
23   libpthread.so.0                    0x00007f16e1d9b6db <unavailable> + 30427
24   libc.so.6                          0x00007f16e041d850 clone + 63
Fatal access conflict detected.

This PR provides a fix for the above crash by creating a shallow copy of delegate ensuring memory safety.

Checklist:

• I have submitted a CLA form
• If applicable, I have updated the documentation accordingly.
• If applicable, I have added tests to cover my changes.

[djones6]

@nethraravindran I'm a little confused. Is this change really making a shallow copy of delegate? It looks to me like it creates a strong reference to the weak var delegate (CurlInvokerDelegate is a class protocol).

[djones6]

The variable delegate is weak, but is being passed as an inout parameter to withUnsafeMutablePointer. (It has to be mutable because it gets passed to the curlHelperSetOptReadFunc et. al functions to set up the curl callbacks.)

So is the issue that, because it is weak, it could be deinitialized before the write access completes?
Or is it because the class var delegate is being accessed within the closure that mutates it?

The var is weak for a reason: to avoid a strong reference cycle between ClientRequest and CurlInvokerDelegate. With this change, for the duration of the invoke function, there will be a strong reference cycle. Is that okay? It sounds like it could actually be a good thing (if ClientRequest was deallocated while a curl request was outstanding, the curl callbacks could try to write to deallocated memory, leading to a crash). Although I don't think that could happen in practice since curl is blocking.

[djones6]

OK, so if it's the latter (delegate is accessed within the closure that mutates it) then shouldn't this always fail? Perhaps this is down to the vagaries of dynamic enforcement (SE-0176 implies dynamic may have to be used when the variable is captured in a closure).

I guess I'm happy with this, I'd be happier if I understood what was going on better, but since this fixes a problem and I can't see evidence of any downside, I'm in.