NSTask and try!

Development Core Libraries
1

Following on from a previous discussion with Tests failing on OSX. I have been looking into the failures. It seems that one of the earliest failures is due to an error from a try! within NSTask.launch(). This came in with this commit: [NSTask] Change argv[0] to full path + fixes combining stdout+stderr … · apple/swift-corelibs-foundation@4c6f04c · GitHub

Going by the docs for Foundation - The launch function apparently "Raises an NSInvalidArgumentException if the launch path has not been set or is invalid or if it fails to create a process."

https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Classes/NSTask_Class/#//apple_ref/occ/instm/NSTask/launch

My question is, should this be built into the Swift Foundation API? The documentation for Swift doesn't state that the launch function throws.

With the test that is failing expecting an error, it feels more Swift-y to have any errors throw explicitly, rather than looking at what the lower level fills the data with.

But before jumping into doing this, I would rather put it out there and see what the community feels about this?

Cheers

James

2

Hi James,

Following on from a previous discussion with Tests failing on OSX. I have been looking into the failures. It seems that one of the earliest failures is due to an error from a try! within NSTask.launch(). This came in with this commit: [NSTask] Change argv[0] to full path + fixes combining stdout+stderr … · apple/swift-corelibs-foundation@4c6f04c · GitHub

Going by the docs for Foundation - The launch function apparently "Raises an NSInvalidArgumentException if the launch path has not been set or is invalid or if it fails to create a process."

https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Classes/NSTask_Class/#//apple_ref/occ/instm/NSTask/launch

My question is, should this be built into the Swift Foundation API? The documentation for Swift doesn't state that the launch function throws.

With the test that is failing expecting an error, it feels more Swift-y to have any errors throw explicitly, rather than looking at what the lower level fills the data with.

But before jumping into doing this, I would rather put it out there and see what the community feels about this?

Unfortunately the ‘throws’ syntax in Swift often causes a mixup between two different things, because it flipped the terminology from what all of our documentation and header comments use.

1. Cocoa uses exceptions (@throw in ObjC) to indicate programmer errors and they are generally not intended to be recoverable. Example: passing nil where not expected, passing an invalid argument, failing to meet a precondition of an API.
2. Cocoa uses NSError ** to indicate runtime errors that are recoverable or at least presentable to user. Example: out of disk space, name of file already exists.

The ‘throws’ syntax in Swift is actually for case #2, not #1. In Swift, #1 is fatalError or preconditionFailure. #2 is ‘throw Error’.

In the case of NSTask, when the documentation says “raises an NSInvalidArgumentException” (#1) then in Swift, that should translate to fatalError or preconditionFailure.

Hope this helps,
- Tony

···

On May 13, 2016, at 12:25 PM, James Lee via swift-corelibs-dev <swift-corelibs-dev@swift.org> wrote:

Cheers

James
_______________________________________________
swift-corelibs-dev mailing list
swift-corelibs-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-corelibs-dev

3

Cheers for the clarification. I started assuming there may be a reason when changing the guard let on the launch args to use the InvalidArgumentException.

Could this be a position where we may need os checking to cover the regression for the moment. It seems odd that the test would pass in CI when an error is thrown with a try! but fail on OSX

···

Sent from my iPhone

On 13 May 2016, at 20:48, Tony Parker <anthony.parker@apple.com> wrote:

Hi James,

On May 13, 2016, at 12:25 PM, James Lee via swift-corelibs-dev <swift-corelibs-dev@swift.org> wrote:

Following on from a previous discussion with Tests failing on OSX. I have been looking into the failures. It seems that one of the earliest failures is due to an error from a try! within NSTask.launch(). This came in with this commit: [NSTask] Change argv[0] to full path + fixes combining stdout+stderr … · apple/swift-corelibs-foundation@4c6f04c · GitHub

Going by the docs for Foundation - The launch function apparently "Raises an NSInvalidArgumentException if the launch path has not been set or is invalid or if it fails to create a process."

https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Classes/NSTask_Class/#//apple_ref/occ/instm/NSTask/launch

My question is, should this be built into the Swift Foundation API? The documentation for Swift doesn't state that the launch function throws.

With the test that is failing expecting an error, it feels more Swift-y to have any errors throw explicitly, rather than looking at what the lower level fills the data with.

But before jumping into doing this, I would rather put it out there and see what the community feels about this?

Unfortunately the ‘throws’ syntax in Swift often causes a mixup between two different things, because it flipped the terminology from what all of our documentation and header comments use.

1. Cocoa uses exceptions (@throw in ObjC) to indicate programmer errors and they are generally not intended to be recoverable. Example: passing nil where not expected, passing an invalid argument, failing to meet a precondition of an API.
2. Cocoa uses NSError ** to indicate runtime errors that are recoverable or at least presentable to user. Example: out of disk space, name of file already exists.

The ‘throws’ syntax in Swift is actually for case #2, not #1. In Swift, #1 is fatalError or preconditionFailure. #2 is ‘throw Error’.

In the case of NSTask, when the documentation says “raises an NSInvalidArgumentException” (#1) then in Swift, that should translate to fatalError or preconditionFailure.

Hope this helps,
- Tony

Cheers

James
_______________________________________________
swift-corelibs-dev mailing list
swift-corelibs-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-corelibs-dev

4

Cheers for the clarification. I started assuming there may be a reason when changing the guard let on the launch args to use the InvalidArgumentException.

Could this be a position where we may need os checking to cover the regression for the moment. It seems odd that the test would pass in CI when an error is thrown with a try! but fail on OSX

Task is certainly one of the cases where the underlying stuff that we’re abstracting is significantly different, so I’m not too surprised.

We should try to get something in place so we’re not failing on OS X in the short term for sure.

- Tony

···

On May 13, 2016, at 1:05 PM, James Lee <james@jelee.co.uk> wrote:

Sent from my iPhone

On 13 May 2016, at 20:48, Tony Parker <anthony.parker@apple.com> wrote:

Hi James,

On May 13, 2016, at 12:25 PM, James Lee via swift-corelibs-dev <swift-corelibs-dev@swift.org> wrote:

Following on from a previous discussion with Tests failing on OSX. I have been looking into the failures. It seems that one of the earliest failures is due to an error from a try! within NSTask.launch(). This came in with this commit: [NSTask] Change argv[0] to full path + fixes combining stdout+stderr … · apple/swift-corelibs-foundation@4c6f04c · GitHub

Going by the docs for Foundation - The launch function apparently "Raises an NSInvalidArgumentException if the launch path has not been set or is invalid or if it fails to create a process."

NSTask | Apple Developer Documentation

My question is, should this be built into the Swift Foundation API? The documentation for Swift doesn't state that the launch function throws.

With the test that is failing expecting an error, it feels more Swift-y to have any errors throw explicitly, rather than looking at what the lower level fills the data with.

But before jumping into doing this, I would rather put it out there and see what the community feels about this?

Unfortunately the ‘throws’ syntax in Swift often causes a mixup between two different things, because it flipped the terminology from what all of our documentation and header comments use.

1. Cocoa uses exceptions (@throw in ObjC) to indicate programmer errors and they are generally not intended to be recoverable. Example: passing nil where not expected, passing an invalid argument, failing to meet a precondition of an API.
2. Cocoa uses NSError ** to indicate runtime errors that are recoverable or at least presentable to user. Example: out of disk space, name of file already exists.

The ‘throws’ syntax in Swift is actually for case #2, not #1. In Swift, #1 is fatalError or preconditionFailure. #2 is ‘throw Error’.

In the case of NSTask, when the documentation says “raises an NSInvalidArgumentException” (#1) then in Swift, that should translate to fatalError or preconditionFailure.

Hope this helps,
- Tony

Cheers

James
_______________________________________________
swift-corelibs-dev mailing list
swift-corelibs-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-corelibs-dev

5

The failing testcase is TestNSTask.test_pipe_stdout_and_stderr_same_pipe. The call to posix_spawn returns an error code 9 (EBADF).

In order to avoid code repetition I've wrapped all posix calls with a throwing status code check;

private func posix(_ code: Int32) throws {
    switch code {
    case 0: return
    default: throw NSError(domain: NSPOSIXErrorDomain, code: Int(code), userInfo: nil)
    }
}

However this produces the not-so-helpful error dump on OSX:

Test Case 'TestNSTask.test_pipe_stdout_and_stderr_same_pipe' started at 10:20:59.741
fatal error: 'try!' expression unexpectedly raised an error: <NSError: 0x0000600000067c40>: file /Users/buildnode/jenkins/workspace/oss-swift-package-osx/swift/stdlib/public/core/ErrorType.swift, line 53

Cheers for the clarification. I started assuming there may be a reason when changing the guard let on the launch args to use the InvalidArgumentException.

Could this be a position where we may need os checking to cover the regression for the moment. It seems odd that the test would pass in CI when an error is thrown with a try! but fail on OSX

Task is certainly one of the cases where the underlying stuff that we’re abstracting is significantly different, so I’m not too surprised.

We should try to get something in place so we’re not failing on OS X in the short term for sure.

- Tony

Sent from my iPhone

Hi James,

Following on from a previous discussion with Tests failing on OSX. I have been looking into the failures. It seems that one of the earliest failures is due to an error from a try! within NSTask.launch(). This came in with this commit: [NSTask] Change argv[0] to full path + fixes combining stdout+stderr … · apple/swift-corelibs-foundation@4c6f04c · GitHub

Going by the docs for Foundation - The launch function apparently "Raises an NSInvalidArgumentException if the launch path has not been set or is invalid or if it fails to create a process."

NSTask | Apple Developer Documentation

My question is, should this be built into the Swift Foundation API? The documentation for Swift doesn't state that the launch function throws.

With the test that is failing expecting an error, it feels more Swift-y to have any errors throw explicitly, rather than looking at what the lower level fills the data with.

But before jumping into doing this, I would rather put it out there and see what the community feels about this?

Unfortunately the ‘throws’ syntax in Swift often causes a mixup between two different things, because it flipped the terminology from what all of our documentation and header comments use.

1. Cocoa uses exceptions (@throw in ObjC) to indicate programmer errors and they are generally not intended to be recoverable. Example: passing nil where not expected, passing an invalid argument, failing to meet a precondition of an API.
2. Cocoa uses NSError ** to indicate runtime errors that are recoverable or at least presentable to user. Example: out of disk space, name of file already exists.

The ‘throws’ syntax in Swift is actually for case #2, not #1. In Swift, #1 is fatalError or preconditionFailure. #2 is ‘throw Error’.

While API compatibility should be the fore-most goal here, I feel like there's room for improvement here for the API overlays. While in ObjC one has the ability to recover from NSInvalidArgumentException, on Swift this would trap.

In the case of NSTask, when the documentation says “raises an NSInvalidArgumentException” (#1) then in Swift, that should translate to fatalError or preconditionFailure.

As a resort; I propose to change the error wrapper (see https://github.com/apple/swift-corelibs-foundation/pull/362\):

private func posix(_ code: Int32) {
    switch code {
    case 0: return
    case EBADF: fatalError("POSIX command failed with error: \(code) -- EBADF")
    default: fatalError("POSIX command failed with error: \(code)")
    }
}

Which would produce the following –more helpful– error on OSX:

Test Case 'TestNSTask.test_pipe_stdout_and_stderr_same_pipe' started at 10:13:55.584
fatal error: POSIX command failed with error: 9 -- EBADF: file <somedir>/swift-corelibs-foundation/Foundation/NSTask.swift, line 424

···

On 2016-05-13 21:07:59 +0000, Tony Parker via swift-corelibs-dev said:

On May 13, 2016, at 1:05 PM, James Lee <james@jelee.co.uk> wrote:

On 13 May 2016, at 20:48, Tony Parker <anthony.parker@apple.com> wrote:

On May 13, 2016, at 12:25 PM, James Lee via swift-corelibs-dev >>>> <swift-corelibs-dev@swift.org> wrote:

Hope this helps,
- Tony

Cheers

James
_______________________________________________
swift-corelibs-dev mailing list
swift-corelibs-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-corelibs-dev

_______________________________________________
swift-corelibs-dev mailing list
swift-corelibs-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-corelibs-dev

6

This does seem to keep more inline with the current Darwin implementation.

Please excuse my ignorance, I have looked into the POSIX calls, but am I right in assuming that the EBADF is due to the test calling to a file that doesn't exist and that is just how OSX handles this case?

Cheers for the clarification

James

···

Sent from my iPhone

On 14 May 2016, at 09:33, Bouke Haarsma via swift-corelibs-dev <swift-corelibs-dev@swift.org> wrote:

The failing testcase is TestNSTask.test_pipe_stdout_and_stderr_same_pipe. The call to posix_spawn returns an error code 9 (EBADF).

In order to avoid code repetition I've wrapped all posix calls with a throwing status code check;

private func posix(_ code: Int32) throws {

    switch code {

    case 0: return

    default: throw NSError(domain: NSPOSIXErrorDomain, code: Int(code), userInfo: nil)

    }

}

However this produces the not-so-helpful error dump on OSX:

Test Case 'TestNSTask.test_pipe_stdout_and_stderr_same_pipe' started at 10:20:59.741

fatal error: 'try!' expression unexpectedly raised an error: <NSError: 0x0000600000067c40>: file /Users/buildnode/jenkins/workspace/oss-swift-package-osx/swift/stdlib/public/core/ErrorType.swift, line 53

On 2016-05-13 21:07:59 +0000, Tony Parker via swift-corelibs-dev said:

On May 13, 2016, at 1:05 PM, James Lee <james@jelee.co.uk> wrote:

Cheers for the clarification. I started assuming there may be a reason when changing the guard let on the launch args to use the InvalidArgumentException.

Could this be a position where we may need os checking to cover the regression for the moment. It seems odd that the test would pass in CI when an error is thrown with a try! but fail on OSX

Task is certainly one of the cases where the underlying stuff that we’re abstracting is significantly different, so I’m not too surprised.

We should try to get something in place so we’re not failing on OS X in the short term for sure.

- Tony

Sent from my iPhone

On 13 May 2016, at 20:48, Tony Parker <anthony.parker@apple.com> wrote:

Hi James,

On May 13, 2016, at 12:25 PM, James Lee via swift-corelibs-dev <swift-corelibs-dev@swift.org> wrote:

Following on from a previous discussion with Tests failing on OSX. I have been looking into the failures. It seems that one of the earliest failures is due to an error from a try! within NSTask.launch(). This came in with this commit: [NSTask] Change argv[0] to full path + fixes combining stdout+stderr … · apple/swift-corelibs-foundation@4c6f04c · GitHub

Going by the docs for Foundation - The launch function apparently "Raises an NSInvalidArgumentException if the launch path has not been set or is invalid or if it fails to create a process."

NSTask | Apple Developer Documentation

My question is, should this be built into the Swift Foundation API? The documentation for Swift doesn't state that the launch function throws.

With the test that is failing expecting an error, it feels more Swift-y to have any errors throw explicitly, rather than looking at what the lower level fills the data with.

But before jumping into doing this, I would rather put it out there and see what the community feels about this?

Unfortunately the ‘throws’ syntax in Swift often causes a mixup between two different things, because it flipped the terminology from what all of our documentation and header comments use.

1. Cocoa uses exceptions (@throw in ObjC) to indicate programmer errors and they are generally not intended to be recoverable. Example: passing nil where not expected, passing an invalid argument, failing to meet a precondition of an API.

2. Cocoa uses NSError ** to indicate runtime errors that are recoverable or at least presentable to user. Example: out of disk space, name of file already exists.

The ‘throws’ syntax in Swift is actually for case #2, not #1. In Swift, #1 is fatalError or preconditionFailure. #2 is ‘throw Error’.

While API compatibility should be the fore-most goal here, I feel like there's room for improvement here for the API overlays. While in ObjC one has the ability to recover from NSInvalidArgumentException, on Swift this would trap.

In the case of NSTask, when the documentation says “raises an NSInvalidArgumentException” (#1) then in Swift, that should translate to fatalError or preconditionFailure.

As a resort; I propose to change the error wrapper (see https://github.com/apple/swift-corelibs-foundation/pull/362\):

private func posix(_ code: Int32) {

    switch code {

    case 0: return

    case EBADF: fatalError("POSIX command failed with error: \(code) -- EBADF")

    default: fatalError("POSIX command failed with error: \(code)")

    }

}

Which would produce the following –more helpful– error on OSX:

Test Case 'TestNSTask.test_pipe_stdout_and_stderr_same_pipe' started at 10:13:55.584

fatal error: POSIX command failed with error: 9 -- EBADF: file <somedir>/swift-corelibs-foundation/Foundation/NSTask.swift, line 424

Hope this helps,

- Tony

Cheers

James

_______________________________________________

swift-corelibs-dev mailing list

swift-corelibs-dev@swift.org

https://lists.swift.org/mailman/listinfo/swift-corelibs-dev

_______________________________________________

swift-corelibs-dev mailing list

swift-corelibs-dev@swift.org

https://lists.swift.org/mailman/listinfo/swift-corelibs-dev

_______________________________________________
swift-corelibs-dev mailing list
swift-corelibs-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-corelibs-dev

7

The problem on OSX was that closing the same FD in the child process would result in the EBADF. The change in the error handling posted before made the error more visible. The actual fix for OSX was to make sure a single FD was closed only once. See also PR #363 Change error checking in NSTask to not use try! by Bouke · Pull Request #362 · apple/swift-corelibs-foundation · GitHub, especially https://github.com/apple/swift-corelibs-foundation/pull/363/commits/3fd0df28107847b7121f6cb2f823fbad9e2ddcff\.

- Bouke

···

On 2016-05-14 09:05:08 +0000, James Lee via swift-corelibs-dev said:

Please excuse my ignorance, I have looked into the POSIX calls, but am I right in assuming that the EBADF is due to the test calling to a file that doesn't exist and that is just how OSX handles this case?

8

Cheers for the clarification. I will spend some time working through some of the other failures, should help me get to grips with it all :)

James

···

Sent from my iPhone

On 14 May 2016, at 15:02, Bouke Haarsma via swift-corelibs-dev <swift-corelibs-dev@swift.org> wrote:

On 2016-05-14 09:05:08 +0000, James Lee via swift-corelibs-dev said:

Please excuse my ignorance, I have looked into the POSIX calls, but am I right in assuming that the EBADF is due to the test calling to a file that doesn't exist and that is just how OSX handles this case?

The problem on OSX was that closing the same FD in the child process would result in the EBADF. The change in the error handling posted before made the error more visible. The actual fix for OSX was to make sure a single FD was closed only once. See also PR #363 Change error checking in NSTask to not use try! by Bouke · Pull Request #362 · apple/swift-corelibs-foundation · GitHub, especially https://github.com/apple/swift-corelibs-foundation/pull/363/commits/3fd0df28107847b7121f6cb2f823fbad9e2ddcff\.

- Bouke

_______________________________________________
swift-corelibs-dev mailing list
swift-corelibs-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-corelibs-dev