[android] Stop leaking FDs in parent test process. #24521
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
compnerd
reviewed
May 6, 2019
In the Android paths of the spawnChild function, the parent was creating a pipe that was never closed, which led to FD starvation. In some tests with a lots of expected crashes, the childs will not spawn anymore since the linker would not have enough descriptors to open the shared libraries, while in other tests which closed the child descriptors as part of the last test, the parent process will hang waiting those descriptors to be closed, which will never had happened. The solution is implement the missing parts of the code, which tried to read from the pipe in the parent side (using select and read, taking pieces from other parts of the code). This should match the fork/execv path used by Android and Haiku to the spawn code used by the rest of the platforms. This change fixes StdlibUnittest/Stdin.swift, stdlib/InputStream.swift.gyb, stdlib/Collection/FlattenCollection.swift.gyb and stdlib/Collection/LazyFilterCollection.swift.gyb, which were the last 4 tests failing in Android AArch64.
drodriguez
force-pushed
the
android-stop-leaking-fds
branch
from
094ad6e to
7d0b78d
Compare
|
Updated incorporating the provided feedback. @swift-ci please test |
drodriguez
added a commit
to drodriguez/swift
that referenced
this pull request
May 7, 2019
Some tests are limited to only Linux, when they should also pass for Android. Additionally, InputStream.swift.gyb was disabled for Android ARMv7, but wasn't for Android AArch64, which allow me to find the error on it and fix it on swiftlang#24521. Finally, StringLowercasedUppercased is interesting in Android because it checks the used ICU is correct for performing the tasks that the stdlib needs.
|
Tested this pull with the latest 5/12 snapshot and can confirm that it fixes those four tests on Android AArch64, the first two of which I've always disabled because they'd always hang. |
compnerd
approved these changes
May 21, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the Android paths of the spawnChild function, the parent was creating
a pipe that was never closed, which led to FD starvation. In some tests
with a lots of expected crashes, the childs will not spawn anymore since
the linker would not have enough descriptors to open the shared
libraries, while in other tests which closed the child descriptors as
part of the last test, the parent process will hang waiting those
descriptors to be closed, which will never had happened.
The solution is implement the missing parts of the code, which tried to
read from the pipe in the parent side (using select and read, taking
pieces from other parts of the code). This should match the fork/execv
path used by Android and Haiku to the spawn code used by the rest of the
platforms.
This change fixes StdlibUnittest/Stdin.swift,
stdlib/InputStream.swift.gyb,
stdlib/Collection/FlattenCollection.swift.gyb and
stdlib/Collection/LazyFilterCollection.swift.gyb, which were the last 4
tests failing in Android AArch64.
/cc @zhuowei which wrote the original code and @return since this code might affect Haiku