Skip to content

Fix nsdata fails to access contents of url #1499

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 4, 2018
Merged

Fix nsdata fails to access contents of url #1499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7dbe8a0
Changed test HTTP server to return response from sub thread
tid-kijyun Apr 1, 2018
481fffe
Fixed nsdata which can not access contents of URL
tid-kijyun Apr 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Foundation/NSData.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ open class NSData : NSObject, NSCopying, NSMutableCopying, NSSecureCoding {
guard let data = resData else {
throw resError!
}
_init(bytes: UnsafeMutableRawPointer(mutating: data._nsObject.bytes), length: length, copy: true)
_init(bytes: UnsafeMutableRawPointer(mutating: data._nsObject.bytes), length: data.count, copy: true)
}
}

Expand Down
2 changes: 1 addition & 1 deletion TestFoundation/HTTPServer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ class _HTTPServer {
deadlineTime = .now()
}

DispatchQueue.main.asyncAfter(deadline: deadlineTime) {
DispatchQueue.global().asyncAfter(deadline: deadlineTime) {
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 split up the data fix itself from the change to the URLSession test code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we can't. Without this change, deadlock will occur in testing this PR. However If you can merge split PRs in order, I should be able to do that.
Would you like to split this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be merged as one PR as the HTTPServer.swift change is required for the test case to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there is anything specific to HTTPServer about the change to NSData. How is it using NSData that triggers this bug?

Copy link
Contributor

@spevans spevans Apr 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If the test case that was added (test_contentsOfURL) is calling NSData(contentsOf: url) and that is running on the main thread then won't the HTTP server have to run on another thread to service the request, or am I mis-understanding how DispatchQueue works? NSData doesn't trigger this bug, its just needed for the unit test.

Copy link
Contributor Author

@tid-kijyun tid-kijyun Apr 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is a code that reproduces this bug.

import Foundation

if let data = NSData(contentsOf: URL(string: "https://swift.org")!) {
    print(data.length) // 0
}

EDIT
As you says, changes to the HTTP server are necessary only for testing.
I add some explanations just in case we need it.

First, NSData(contentsOf:) uses URLSession.

https://github.com/apple/swift-corelibs-foundation/blob/2ba2086405c864e0c8bdb223209cf9a87e74a1d2/Foundation/NSData.swift#L207

Then, NSData(contentsOf:) uses NSCondition wait().
https://github.com/apple/swift-corelibs-foundation/blob/2ba2086405c864e0c8bdb223209cf9a87e74a1d2/Foundation/NSData.swift#L217
While the main thread is blocking with NSCondition wait(), The following test HTTP server code enqueued into the main thread queue is not executed.

https://github.com/apple/swift-corelibs-foundation/blob/2ba2086405c864e0c8bdb223209cf9a87e74a1d2/TestFoundation/HTTPServer.swift#L211-L214

Finally, Since there is no response from test HTTP server, deadlock will occur because the following code to resolve NSData's wait is not executed.

https://github.com/apple/swift-corelibs-foundation/blob/2ba2086405c864e0c8bdb223209cf9a87e74a1d2/Foundation/NSData.swift#L214

So, we need to change the HTTP server for testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thanks for the additional info.

do {
try self.socket.writeData(header: response.header, body: response.body, sendDelay: sendDelay, bodyChunks: bodyChunks)
semaphore.signal()
Expand Down
13 changes: 12 additions & 1 deletion TestFoundation/TestNSData.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
import SwiftXCTest
#endif

class TestNSData: XCTestCase {
class TestNSData: LoopbackServerTest {

class AllOnesImmutableData : NSData {
private var _length : Int
Expand Down Expand Up @@ -201,6 +201,7 @@ class TestNSData: XCTestCase {
("test_openingNonExistentFile", test_openingNonExistentFile),
("test_contentsOfFile", test_contentsOfFile),
("test_contentsOfZeroFile", test_contentsOfZeroFile),
("test_contentsOfURL", test_contentsOfURL),
("test_basicReadWrite", test_basicReadWrite),
("test_bufferSizeCalculation", test_bufferSizeCalculation),
("test_dataHash", test_dataHash),
Expand Down Expand Up @@ -1473,6 +1474,16 @@ extension TestNSData {
#endif
}

func test_contentsOfURL() {
let urlString = "http://127.0.0.1:\(TestURLSession.serverPort)/country.txt"
let url = URL(string: urlString)!
let contents = NSData(contentsOf: url)
XCTAssertNotNil(contents)
if let contents = contents {
XCTAssertTrue(contents.length > 0)
}
}

func test_basicReadWrite() {
let url = URL(fileURLWithPath: NSTemporaryDirectory(), isDirectory: true).appendingPathComponent("testfile")
let count = 1 << 24
Expand Down