8255758: JEP 380 spec clarifications #1021

Michael-Mc-Mahon · 2020-11-02T22:04:56Z

Minor spec changes from spec approved in initial CSR

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Testing

	Linux x64	Linux x86	Windows x64	macOS x64
Build	⏳ (3/5 running)	⏳ (2/2 running)	⏳ (2/2 running)	⏳ (2/2 running)

Issue

JDK-8255758: JEP 380 spec clarifications

Reviewers

Daniel Fuchs (@dfuch - Reviewer) ⚠️ Review applies to 52c1276
Alan Bateman (@AlanBateman - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1021/head:pull/1021
$ git checkout pull/1021

bridgekeeper · 2020-11-02T22:05:58Z

👋 Welcome back michaelm! 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 · 2020-11-02T22:08:13Z

@Michael-Mc-Mahon The following labels will be automatically applied to this pull request:

net
nio

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

dfuch · 2020-11-03T09:07:27Z

src/java.base/share/classes/java/net/doc-files/net-properties.html

+java.nio.channels.ServerSocketChannel#bind(SocketAddress,int) ServerSocketChannel.bind}
+is called with a {@code null} address parameter. In this case, the system


doesn't it also happen when it is called with a UnixDomainSocketAddress that has an empty path?

No, an empty path signifies an unnamed address and only client sockets (SocketChannels) are allowed to be unnamed.

You get a BindException if you try to bind a ServerSocketChannel to the unnamed address. But, the exception message is "unhelpful". I would like to fix that.

Ah good. I missed that paragraph was only talking about the server sockets.

dfuch · 2020-11-03T09:09:18Z

src/java.base/share/classes/java/net/doc-files/net-properties.html

+<a id="Unixdomain"></a>
+<H2>Unix domain sockets</H2>
+<P>There are a number of system (and networking) properties that affect the behavior of
+channels to <i>Unix domain</i> server sockets when binding <i>automatically</i>.


Should this also mention implicit binding?

I'm not sure, as the system properties don't affect implicit binding, but I would have liked to have a place to "explain" all aspects of binding of Unix domain sockets/server-sockets. Maybe, I could add a paragraph at the end, just as an aside or note.

Oh - I was assuming that implicit binding & automatic binding were identical - just had a different trigger. Maybe it would be worth spelling out that implicit binding is not affected by the property then? For my own curiosity - is it because automatic binding is performed by our java implementation while implicit binding is performed by the system?

These concepts aren't new by the way. "Implicit" binding is when you create a client socket and connect it without calling bind first. This is not possible with server sockets.

"Automatic" binding is when you bind a server socket with a null address. All the above is the same for tcp and unix domain. tcp has more options relating to the interface and specifying a port = 0. These cases don't exist for unix domain.

Just to add, one other point. It is also possible to automatically bind a client socket (either tcp or unix) (ie bind(null)) There is a small conceptual difference here between tcp and unix. With tcp you get a random port number, but with unix domain you get an unnamed socket address.

src/java.base/share/classes/java/net/doc-files/net-properties.html

openjdk · 2020-11-03T14:56:09Z

@Michael-Mc-Mahon 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:

8255758: JEP 380 spec clarifications

Reviewed-by: dfuchs, alanb

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 24 new commits pushed to the master branch:

3dcde55: 8256386: ARM32 tests fail with "bad AD file" after JDK-8223051
53a3188: 8202471: (ann) Cannot read type annotations on generic receiver type's type variables
adb8561: 8253228: [JVMCI] provide more info on fatal JVMCI errors
a7422ac: 8255150: Add utility methods to check long indexes and ranges
6d87856: 8256325: Remove duplicate asserts in PhaseMacroExpand::expand_macro_nodes
5dbfae0: 8255058: C1: assert(is_virtual()) failed: type check
4553fa0: 8256258: some missing NULL checks or asserts after CodeCache::find_blob_unsafe
1228517: 8256274: C2: Optimize copying of the shared type dictionary
537b40e: 8252588: HotSpot Style Guide should permit uniform initialization
c35e1a2: 8255947: [macos] Signed macOS jpackage app doesn't filter spurious '-psn' argument
... and 14 more: https://git.openjdk.java.net/jdk/compare/ac3948930ef2b188401d67f20f730fefe7d15e1e...master

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.

Michael-Mc-Mahon · 2020-11-05T10:15:19Z

/csr needed

openjdk · 2020-11-05T10:18:56Z

@Michael-Mc-Mahon has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@Michael-Mc-Mahon please create a CSR request and add link to it in JDK-8255758. This pull request cannot be integrated until the CSR request is approved.

mlbridge · 2020-11-05T10:23:01Z

Webrevs

AlanBateman · 2020-11-15T08:32:28Z

The clarification to the SecurityException looks good.

The API docs specify that bind(null) will "bind to an automatically assigned socket address". Would it be better to lead with that phrase in the properties doc rather than switching to "automatically bound". That might also avoid it getting confused with an implicitly bound sockets.

A minor comment on the test is that "peer" might be a better than "acc1".

Michael-Mc-Mahon · 2020-11-16T13:26:26Z

The clarification to the SecurityException looks good.

The API docs specify that bind(null) will "bind to an automatically assigned socket address". Would it be better to lead with that phrase in the properties doc rather than switching to "automatically bound". That might also avoid it getting confused with an implicitly bound sockets.

A minor comment on the test is that "peer" might be a better than "acc1".

I've just updated the spec per these comments and will update the CSR accordingly. Thx.

src/java.base/share/classes/java/net/doc-files/net-properties.html

AlanBateman · 2020-11-17T11:25:22Z

src/java.base/share/classes/java/net/doc-files/net-properties.html

+Calling {@link java.nio.channels.ServerSocketChannel#bind(SocketAddress,int) ServerSocketChannel.bind}
+with a {@code null} address parameter will bind to an <i>automatically assigned</i> socket address.
+For Unix domain sockets, this means a unique path in some predefined system temporary directory.
+<P>There are a number of system (and networking) properties that affect this behavior.


"There are are a number of system properties ..." follows directly from the previous sentence so I assume you don't want a paragraph break here.

Ok, that's fine.

AlanBateman · 2020-11-17T11:27:40Z

src/java.base/share/classes/java/net/doc-files/net-properties.html

+For Unix domain sockets, this means a unique path in some predefined system temporary directory.
+<P>There are a number of system (and networking) properties that affect this behavior.
+<p>
+Bearing in mind that Unix domain socket addresses are limited in length to approximately 100 


I think "Bearing in mind" should be dropped here and just say that Unix domain sockets are typically limited to a file path of 100 characters (typically it is bytes that may be too subtle for the context).

Michael-Mc-Mahon · 2020-11-17T12:10:52Z

/integrate

openjdk · 2020-11-17T12:12:45Z

@Michael-Mc-Mahon Since your change was applied there have been 24 commits pushed to the master branch:

3dcde55: 8256386: ARM32 tests fail with "bad AD file" after JDK-8223051
53a3188: 8202471: (ann) Cannot read type annotations on generic receiver type's type variables
adb8561: 8253228: [JVMCI] provide more info on fatal JVMCI errors
a7422ac: 8255150: Add utility methods to check long indexes and ranges
6d87856: 8256325: Remove duplicate asserts in PhaseMacroExpand::expand_macro_nodes
5dbfae0: 8255058: C1: assert(is_virtual()) failed: type check
4553fa0: 8256258: some missing NULL checks or asserts after CodeCache::find_blob_unsafe
1228517: 8256274: C2: Optimize copying of the shared type dictionary
537b40e: 8252588: HotSpot Style Guide should permit uniform initialization
c35e1a2: 8255947: [macos] Signed macOS jpackage app doesn't filter spurious '-psn' argument
... and 14 more: https://git.openjdk.java.net/jdk/compare/ac3948930ef2b188401d67f20f730fefe7d15e1e...master

Your commit was automatically rebased without conflicts.

Pushed as commit 9d0ee66.

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

Reviewed-by: dfuchs, alanb

8255758: JEP 380 spec clarifications

3a29893

openjdk bot added nio nio-dev@openjdk.org net net-dev@openjdk.org labels Nov 2, 2020

dfuch reviewed Nov 3, 2020

View reviewed changes

update to net-properties.html

e17e78b

dfuch approved these changes Nov 3, 2020

View reviewed changes

src/java.base/share/classes/java/net/doc-files/net-properties.html Outdated Show resolved Hide resolved

src/java.base/share/classes/java/net/doc-files/net-properties.html Outdated Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label Nov 3, 2020

Michael-Mc-Mahon marked this pull request as ready for review November 5, 2020 10:14

openjdk bot added rfr Pull request is ready for review csr Pull request needs approved CSR before integration labels Nov 5, 2020

openjdk bot removed the ready Pull request is ready to be integrated label Nov 5, 2020

net-properties.html change only. src apidoc change to come

e492c38

Michael-Mc-Mahon added 2 commits November 16, 2020 11:11

Merge branch 'master' into 8255758-spec-change

c2e2119

spec update after Alan's comments

52c1276

dfuch approved these changes Nov 16, 2020

View reviewed changes

src/java.base/share/classes/java/net/doc-files/net-properties.html Outdated Show resolved Hide resolved

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Nov 16, 2020

minor change suggested by Daniel

1f9c1a7

AlanBateman reviewed Nov 17, 2020

View reviewed changes

update from Alan's comment Nov 17

ce3bb6f

AlanBateman approved these changes Nov 17, 2020

View reviewed changes

openjdk bot closed this Nov 17, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 17, 2020

Michael-Mc-Mahon deleted the 8255758-spec-change branch November 17, 2020 12:12

openjdk-notifier bot referenced this pull request Nov 17, 2020

8255758: JEP 380 spec clarifications

9d0ee66

Reviewed-by: dfuchs, alanb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8255758: JEP 380 spec clarifications #1021

8255758: JEP 380 spec clarifications #1021

Michael-Mc-Mahon commented Nov 2, 2020 •

edited by openjdk bot

bridgekeeper bot commented Nov 2, 2020

openjdk bot commented Nov 2, 2020

dfuch Nov 3, 2020

Michael-Mc-Mahon Nov 3, 2020

Michael-Mc-Mahon Nov 3, 2020

dfuch Nov 3, 2020 •

edited

dfuch Nov 3, 2020

Michael-Mc-Mahon Nov 3, 2020

dfuch Nov 3, 2020

Michael-Mc-Mahon Nov 3, 2020 •

edited

openjdk bot commented Nov 3, 2020 •

edited

Michael-Mc-Mahon commented Nov 5, 2020

openjdk bot commented Nov 5, 2020

mlbridge bot commented Nov 5, 2020 •

edited

AlanBateman commented Nov 15, 2020

Michael-Mc-Mahon commented Nov 16, 2020

AlanBateman Nov 17, 2020

Michael-Mc-Mahon Nov 17, 2020

AlanBateman Nov 17, 2020

Michael-Mc-Mahon Nov 17, 2020

Michael-Mc-Mahon commented Nov 17, 2020

openjdk bot commented Nov 17, 2020

		java.nio.channels.ServerSocketChannel#bind(SocketAddress,int) ServerSocketChannel.bind}
		is called with a {@code null} address parameter. In this case, the system

8255758: JEP 380 spec clarifications #1021

8255758: JEP 380 spec clarifications #1021

Conversation

Michael-Mc-Mahon commented Nov 2, 2020 • edited by openjdk bot

Progress

Testing

Issue

Reviewers

Download

bridgekeeper bot commented Nov 2, 2020

openjdk bot commented Nov 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfuch Nov 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael-Mc-Mahon Nov 3, 2020 • edited

Choose a reason for hiding this comment

openjdk bot commented Nov 3, 2020 • edited

Michael-Mc-Mahon commented Nov 5, 2020

openjdk bot commented Nov 5, 2020

mlbridge bot commented Nov 5, 2020 • edited

Webrevs

AlanBateman commented Nov 15, 2020

Michael-Mc-Mahon commented Nov 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael-Mc-Mahon commented Nov 17, 2020

openjdk bot commented Nov 17, 2020

Michael-Mc-Mahon commented Nov 2, 2020 •

edited by openjdk bot

dfuch Nov 3, 2020 •

edited

Michael-Mc-Mahon Nov 3, 2020 •

edited

openjdk bot commented Nov 3, 2020 •

edited

mlbridge bot commented Nov 5, 2020 •

edited