Skip to content

fix tests dying because of SIGPIPE #1099

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.

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 1 commit into from
Jul 7, 2017
Merged

fix tests dying because of SIGPIPE #1099

merged 1 commit into from
Jul 7, 2017

Conversation

weissi
Copy link
Contributor

@weissi weissi commented Jul 6, 2017

(POSIX sends SIGPIPE when writing to a closed fd which will then kill your project unless you ignore it or handle it properly)

@weissi weissi requested review from phausler and pushkarnk July 6, 2017 10:47
@weissi
Copy link
Contributor Author

weissi commented Jul 6, 2017

@swift-ci please test

@alblue
Copy link
Contributor

alblue commented Jul 6, 2017

Looks good to me.

@@ -46,6 +48,8 @@ class TestURLSession : XCTestCase {

override class func setUp() {
super.setUp()

_ = signal(SIGPIPE, SIG_IGN)
Copy link
Contributor

Choose a reason for hiding this comment

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

we might want to add this earlier in the lifecycle (e.g. main.swift) so that it applies equally to all tests (perhaps we might add more in the future that will need this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@phausler agreed, good idea! Will update.

@weissi
Copy link
Contributor Author

weissi commented Jul 6, 2017

@phausler updated

@weissi
Copy link
Contributor Author

weissi commented Jul 6, 2017

@swift-ci please test

(`POSIX` sends `SIGPIPE` when writing to a closed fd)
@weissi
Copy link
Contributor Author

weissi commented Jul 6, 2017

@swift-ci please test

1 similar comment
@weissi
Copy link
Contributor Author

weissi commented Jul 6, 2017

@swift-ci please test

Copy link
Contributor

@phausler phausler left a comment

Choose a reason for hiding this comment

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

looks good to me

@spevans
Copy link
Contributor

spevans commented Jul 6, 2017

was there a specific situation where a closed fd was being written to or is it just to cover potential future issues?

@weissi
Copy link
Contributor Author

weissi commented Jul 6, 2017

@spevans there was a test that was failing because of that, I think test_writeSomething, can't really remember the name.

Copy link
Member

@pushkarnk pushkarnk left a comment

Choose a reason for hiding this comment

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

LGTM

@pushkarnk
Copy link
Member

@swift-ci please test and merge

@pushkarnk
Copy link
Member

@swift-ci test and merge

@swift-ci swift-ci merged commit 0bfc8d6 into swiftlang:master Jul 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants