Skip to content
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

8291067: macOS should use O_CLOEXEC instead of FD_CLOEXEC #9663

Closed
wants to merge 11 commits into from
Closed

8291067: macOS should use O_CLOEXEC instead of FD_CLOEXEC #9663

wants to merge 11 commits into from

Conversation

gerard-ziemski
Copy link

@gerard-ziemski gerard-ziemski commented Jul 27, 2022

O_CLOEXEC is also available on macOS (__DARWIN_C_LEVEL >= 200809L), so use it same as on linux.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8291067: macOS should use O_CLOEXEC instead of FD_CLOEXEC

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9663/head:pull/9663
$ git checkout pull/9663

Update a local copy of the PR:
$ git checkout pull/9663
$ git pull https://git.openjdk.org/jdk pull/9663/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9663

View PR using the GUI difftool:
$ git pr show -t 9663

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9663.diff

@gerard-ziemski gerard-ziemski marked this pull request as ready for review July 27, 2022 19:28
@bridgekeeper
Copy link

bridgekeeper bot commented Jul 27, 2022

👋 Welcome back gziemski! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Jul 27, 2022
@openjdk
Copy link

openjdk bot commented Jul 27, 2022

@gerard-ziemski The following label will be automatically applied to this pull request:

  • hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Jul 27, 2022
@mlbridge
Copy link

mlbridge bot commented Jul 27, 2022

Webrevs

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Choose a reason for hiding this comment

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

Thumbs up. What Mach5 testing have you done here?

Comment on lines 2211 to 2215
// Modern Linux kernels (after 2.6.23 2007) support O_CLOEXEC with open().
// O_CLOEXEC is preferable to using FD_CLOEXEC on an open file descriptor
// because it saves a system call and removes a small window where the flag
// is unset. On ancient Linux kernels the O_CLOEXEC flag will be ignored
// and we fall back to using FD_CLOEXEC (see below).
Copy link
Member

Choose a reason for hiding this comment

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

This comment was confusing me until I looked at os_linux.cpp and see
that you are getting in sync with the comments in that file (which is
where this code was ported from).

Your changes below are also getting in sync with the code in the
os_linux.cpp file.

Did you want to mention here the macOS version info that includes
the support for O_CLOEXEC?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the review Dan!

I thought about it, but I decided that keeping the code in sync between linux and macOS was the priority. Eventually we might want to have a single impl shared between linux and mac I thought.

I ran Mach5 hs-tier1...5 for the testing.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry but copying a comment from os_linux to os_bsd really doesn't make sense. We've been cleaning up these "mis-ports" ever since the macOS port was done, so please don't add new occurrences.

Copy link
Member

Choose a reason for hiding this comment

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

I see now it is just not comments but also the code. Again sorry but if the Linux workarounds are for Linux then they don't belong in the BSD code. That just confuses anyone who looks at this code later. If there were eventually an os_posix shared version of this then it wouldn't include Linux specific workarounds.

Copy link
Author

Choose a reason for hiding this comment

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

This is not a workaround but an improvement to the robustness of the code, that applies equally to both Linux and macOS, by taking an advantage of what once was a new feature, but now is just a more reliable mechanism for making sure that the forked/child process does not get access to unintended "leaked" file descriptor from the parent. The original comment came from Linux, but we can change it, to apply equally to both Linux/macOS.

I will refactor this common POSIX code, though they are not 100% exactly the same - Linux uses open64(), while macOS uses open().

Copy link
Member

Choose a reason for hiding this comment

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

The comment says:

On ancient Linux kernels the O_CLOEXEC flag will be ignored and we fall back to using FD_CLOEXEC (see below).

so that is a workaround to handle ancient Linux kernels, and such a workaround has no meaning for BSD unless it too has ancient versions that ignored O_CLOEXEC.

That said, how ancient are we talking about? That comment may be irrelevant today given the minimum supported kernel versions we have.

Copy link
Author

Choose a reason for hiding this comment

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

I have tested O_CLOEXEC on macOS and it is available even at 10.9 I don't have hardware that can go earlier than that.

So it looks like on macOS we don't need the fallback, however, macOS is not the only BSD OS that we need to consider (?)

Should we leave the fallback for other BSD platforms?

Copy link
Member

Choose a reason for hiding this comment

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

Is there anything to suggest any BSD system has ever had this issue? This seems a Linux specific workaround that has no place on macOS or BSD.

Copy link
Author

Choose a reason for hiding this comment

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

If you look at fcntl.h you will find:

#if __DARWIN_C_LEVEL >= 200809L
#define O_CLOEXEC       0x01000000      /* implicitly set FD_CLOEXEC */
#endif

so that suggests that O_CLOEXEC was not always available on macOS, so I assume same holds true for other BSD os'es?

We could tweak the BSD code to statically account for the availability of O_CLOEXEC with #ifdef , but why not re-use Linux code and do it dynamically as needed?

On Linux this accounts for the real possibility of kernel ignoring O_CLOEXEC and on BSD the workaround would be a actually a fallback on those platform that plain did not support O_CLOEXEC at all.

We could tweak the comment to reflect these assumptions.

Copy link
Member

Choose a reason for hiding this comment

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

Hi Gerard,

sorry, I'm with @dholmes-ora on this one. Copying Linux comments like this is not really helpful.

  • Completely different kernels (both BSD and Mac), therefore I would skip the "did the kernel ignore my flag?" check. That was a workaround for one very specific Linux kernel bug.
  • The #ifdef O_CLOEXEC does not really help you with backward compatibility if it is really your aim to be runnable on older OSes. Since this tests on the build machine, kernel on the runtime machine may not understand (hopefully ignore) the numerical value O_CLOEXEC.

Honestly, I would not bother. I would just use open with O_CLOEXEC and be done with it. If someone really really wants to build JDK 20 on ancient MacOS or BSD, then we can add complexity.

Just my 5 cent.

Cheers, Thomas

@openjdk
Copy link

openjdk bot commented Jul 28, 2022

@gerard-ziemski This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8291067: macOS should use O_CLOEXEC instead of FD_CLOEXEC

Reviewed-by: dcubed, dholmes, stuefe

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 3 new commits pushed to the master branch:

  • 392ac70: 8297211: Expensive fillInStackTrace operation in HttpURLConnection.getOutputStream0 when no content-length in response
  • 5a45c25: 8297164: Update troff man pages and CheckManPageOptions.java
  • f12710e: 8288047: Accelerate Poly1305 on x86_64 using AVX512 instructions

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jul 28, 2022
Comment on lines 2211 to 2215
// Modern Linux kernels (after 2.6.23 2007) support O_CLOEXEC with open().
// O_CLOEXEC is preferable to using FD_CLOEXEC on an open file descriptor
// because it saves a system call and removes a small window where the flag
// is unset. On ancient Linux kernels the O_CLOEXEC flag will be ignored
// and we fall back to using FD_CLOEXEC (see below).
Copy link
Member

Choose a reason for hiding this comment

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

Sorry but copying a comment from os_linux to os_bsd really doesn't make sense. We've been cleaning up these "mis-ports" ever since the macOS port was done, so please don't add new occurrences.

Comment on lines 2211 to 2215
// Modern Linux kernels (after 2.6.23 2007) support O_CLOEXEC with open().
// O_CLOEXEC is preferable to using FD_CLOEXEC on an open file descriptor
// because it saves a system call and removes a small window where the flag
// is unset. On ancient Linux kernels the O_CLOEXEC flag will be ignored
// and we fall back to using FD_CLOEXEC (see below).
Copy link
Member

Choose a reason for hiding this comment

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

I see now it is just not comments but also the code. Again sorry but if the Linux workarounds are for Linux then they don't belong in the BSD code. That just confuses anyone who looks at this code later. If there were eventually an os_posix shared version of this then it wouldn't include Linux specific workarounds.

Comment on lines 2224 to 2239
{
struct stat buf;
int ret = ::fstat(fd, &buf);
int st_mode = buf.st_mode;

if (ret != -1) {
if ((st_mode & S_IFMT) == S_IFDIR) {
errno = EISDIR;
::close(fd);
return -1;
}
} else {
::close(fd);
return -1;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? This is a behavioral change that has nothing to do with the bug description.

Comment on lines 2211 to 2215
// Modern Linux kernels (after 2.6.23 2007) support O_CLOEXEC with open().
// O_CLOEXEC is preferable to using FD_CLOEXEC on an open file descriptor
// because it saves a system call and removes a small window where the flag
// is unset. On ancient Linux kernels the O_CLOEXEC flag will be ignored
// and we fall back to using FD_CLOEXEC (see below).
Copy link
Member

Choose a reason for hiding this comment

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

Hi Gerard,

sorry, I'm with @dholmes-ora on this one. Copying Linux comments like this is not really helpful.

  • Completely different kernels (both BSD and Mac), therefore I would skip the "did the kernel ignore my flag?" check. That was a workaround for one very specific Linux kernel bug.
  • The #ifdef O_CLOEXEC does not really help you with backward compatibility if it is really your aim to be runnable on older OSes. Since this tests on the build machine, kernel on the runtime machine may not understand (hopefully ignore) the numerical value O_CLOEXEC.

Honestly, I would not bother. I would just use open with O_CLOEXEC and be done with it. If someone really really wants to build JDK 20 on ancient MacOS or BSD, then we can add complexity.

Just my 5 cent.

Cheers, Thomas

@tstuefe
Copy link
Member

tstuefe commented Aug 26, 2022

What you could do and what would be helpful is a very primitive gtest that tests that files opened with os::open are correctly tagged with FD_CLOEXEC.

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 23, 2022

@gerard-ziemski This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@gerard-ziemski
Copy link
Author

What you could do and what would be helpful is a very primitive gtest that tests that files opened with os::open are correctly tagged with FD_CLOEXEC.

I like that.

@gerard-ziemski
Copy link
Author

Followed feedback from David and Thomas.

We now assume that O_CLOEXEC is available on macOS, but we added __APPLE__ gtest that verifies that assumption via:

make run-test TEST=gtest:os.open_O_CLOEXEC

@@ -887,3 +887,13 @@ TEST_VM(os, is_first_C_frame) {
EXPECT_FALSE(os::is_first_C_frame(&cur_frame));
#endif // _WIN32
}

TEST_VM(os, open_O_CLOEXEC) {
#if defined(__APPLE__)
Copy link
Member

Choose a reason for hiding this comment

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

Does this not work on other OS ie Linux?

Copy link
Author

Choose a reason for hiding this comment

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

It works on Linux too and originally I had it for both Mac and Linux, but I figured that since this issue (dropping FD_CLOEXEC and relaying on only O_CLOEXEC) is focused just on MacOS, then that's the platform we should test.

I can add Linux here though, if you think it would be useful.

Copy link
Member

Choose a reason for hiding this comment

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

This should work for all Unices. Can you test it for all !windows platforms?

Also, thanks for adding the test. The reason I did ask for it was that we have that insanely complicated test/hotspot/jtreg/runtime/8176717/TestInheritFD.java in the jtreg suite that checks whether UL log files are inherited to child processes (so, not marked with CLOEXEC). I always wondered whether we could replace that one with just a very simple gtest, that tests that UL opens its files with CLOEXEC.

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 10, 2022

@gerard-ziemski This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

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

Some small remarks inline, otherwise this looks ok

src/hotspot/os/bsd/os_bsd.cpp Outdated Show resolved Hide resolved
src/hotspot/os/bsd/os_bsd.cpp Outdated Show resolved Hide resolved
@@ -887,3 +887,13 @@ TEST_VM(os, is_first_C_frame) {
EXPECT_FALSE(os::is_first_C_frame(&cur_frame));
#endif // _WIN32
}

TEST_VM(os, open_O_CLOEXEC) {
#if defined(__APPLE__)
Copy link
Member

Choose a reason for hiding this comment

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

This should work for all Unices. Can you test it for all !windows platforms?

Also, thanks for adding the test. The reason I did ask for it was that we have that insanely complicated test/hotspot/jtreg/runtime/8176717/TestInheritFD.java in the jtreg suite that checks whether UL log files are inherited to child processes (so, not marked with CLOEXEC). I always wondered whether we could replace that one with just a very simple gtest, that tests that UL opens its files with CLOEXEC.

@gerard-ziemski
Copy link
Author

Thank you Thomas for more feedback. Hopefully I have resolved all your comments.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

This seems fine to me now. Thanks

Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

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

Good!

@gerard-ziemski
Copy link
Author

Thank you Dan, David and Thomas for your feedback!

@gerard-ziemski
Copy link
Author

Hopefully this is it. Please take a look, thank you!

@tstuefe
Copy link
Member

tstuefe commented Nov 17, 2022

Looks good. Ship it!

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Choose a reason for hiding this comment

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

Having the open() call do the create is simpler.
I don't know about using "test" for the file name.

@gerard-ziemski
Copy link
Author

Having the open() call do the create is simpler.
I don't know about using "test" for the file name.

Would you like me to change it? Would test_file.txt work or did you have something else in mind?

@dcubed-ojdk
Copy link
Member

test_file.txt would be good (if you don't mind).

…ose of the artifact to however happens to be looking
@gerard-ziemski
Copy link
Author

Would love to ship this one.

@tstuefe
Copy link
Member

tstuefe commented Nov 18, 2022

Still good.

@openjdk
Copy link

openjdk bot commented Nov 21, 2022

@gerard-ziemski this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8291067
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 21, 2022
@openjdk openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review and removed merge-conflict Pull request has merge conflict with target branch labels Nov 21, 2022
@gerard-ziemski
Copy link
Author

The fix had merge conflict, which I had to cleanup and now everyone who reviewed it is shown as "reviewer of previous version". Am I fine to go ahead with the integration, or do I need your final OK?

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Choose a reason for hiding this comment

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

Since it has not added a "ready" in the time since you resolved the
merge conflicts, I'm guessing it wants at least one re-review.

Thumbs up.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Still good

@tstuefe
Copy link
Member

tstuefe commented Nov 22, 2022

+1

@gerard-ziemski
Copy link
Author

Thank you all very much for your patience on this one! I really do appreciate it.

@gerard-ziemski
Copy link
Author

/integrate

@openjdk
Copy link

openjdk bot commented Nov 22, 2022

Going to push as commit ccc6e16.
Since your change was applied there have been 10 commits pushed to the master branch:

  • 0ac5b55: 8297349: Parallel: Use correct claim value for CLD oop iteration in PSScavengeCLDClosure
  • 932bf35: 8297333: Parallel: Remove unused methods in PCIterateMarkAndPushClosure
  • 42c2037: 8297382: Test fails to compile after JDK-8288047
  • 6d6046b: 8252713: jtreg time out of CtrlASCII.java seems to hang the Xserver.
  • 88957a7: 8297147: UnexpectedSourceImageSize test times out on slow machines when fastdebug is used
  • 0696854: 8297299: SequenceInputStream should not use Vector
  • f0e99c6: 8297301: Cleanup unused methods in JavaUtilJarAccess
  • 392ac70: 8297211: Expensive fillInStackTrace operation in HttpURLConnection.getOutputStream0 when no content-length in response
  • 5a45c25: 8297164: Update troff man pages and CheckManPageOptions.java
  • f12710e: 8288047: Accelerate Poly1305 on x86_64 using AVX512 instructions

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 22, 2022
@openjdk openjdk bot closed this Nov 22, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 22, 2022
@openjdk
Copy link

openjdk bot commented Nov 22, 2022

@gerard-ziemski Pushed as commit ccc6e16.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
4 participants