-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Add 0 to SIG enum #26012
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
Closed
+43
−0
Closed
Add 0 to SIG enum #26012
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| const std = @import("std"); | ||
| const posix = std.posix; | ||
| const builtin = @import("builtin"); | ||
| const native_os = builtin.target.os.tag; | ||
|
|
||
| pub fn main() !void { | ||
| try test_kill_zero_self_should_succeed(); | ||
| try test_kill_nonexistent(); | ||
| } | ||
|
|
||
| fn test_kill_nonexistent() !void { | ||
| if ((native_os != .linux) and (native_os != .macos)) return; | ||
| // Linux is limited by PID_MAX_LIMIT constant which is around 4 million | ||
| // MacOS maximum pid appears to be 99999 and not configurable. | ||
| // Others are unknown thus not tested. | ||
| const impossible_pid: posix.pid_t = 1_999_999_999; | ||
| try std.testing.expectError(posix.KillError.ProcessNotFound, posix.kill(impossible_pid, .INVAL)); | ||
| } | ||
|
|
||
| fn test_kill_zero_self_should_succeed() !void { | ||
| // Windows does not have kill -0 equivalent | ||
| if (native_os == .windows) return; | ||
| try posix.kill(posix.getpid(), .INVAL); | ||
| } | ||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All it takes is a write to
/proc/sys/kernel/pid_maxon Linux to make this test unreliable.Unless you can come up with a non-flaky and portable way of obtaining a truly "impossible" PID, this part of the test can't be merged.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might be wrong. There appears to be a constant in linux PID_MAX_LIMIT which limits that number to somewhere around 4 million. https://github.com/torvalds/linux/blob/master/include/linux/threads.h#L34
Attempting
# echo "4194305" > /proc/sys/kernel/pid_maxconfirms that, at least for linux. I am not knowledgable of other posix systems, but even then it seems better to have this test that is extremely unlikely to ever be flakey, than to not have the test at all?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can have the test if it's only enabled for operating systems on which it can be verified that the PID is in fact impossible. We have a pretty strict "no flaky tests" policy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have limited the test to linux (limited by the constant) and macos (results of searching generally agree on a non-configurable and verifiable number)
I have also excluded windows from the test because it apparently does not have
kill -0equivalentThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this is testing Zig's handling of the error path (vs. testing the OS's error path), it seems sufficient (and safe) to make this a
(native_os == .linux)only test. Then you can be confident in thePID_MAX_LIMIT, and we exercise the Zig code well enough?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, github didn't show me your most recent update, which basically does what I suggested, plus MacOS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion, I have already limited to linux and macos. both of which I am confident of never being able to hit the PID limit, if that is fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, same :D That is resolved than.