Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
On Tue, 21 Sep 2021 17:16:08 GMT, Michael McMahon wrote: >> Julia Boes has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 16 commits: >> >> - Merge branch 'master' into simpleserver >> - Merge remote-tracking branch 'origin/simpleserver' into simpleserver >> - Merge branch 'master' into simpleserver >> - refactor isHidden,isReadable,isSymlink checks and cleanup tests >> - Merge branch 'master' into simpleserver >> - check isHidden, isSymlink, isReadable for all path segments >> - add checks for all path segments >> - Merge branch 'master' into componentcheck >> - Merge branch 'master' into simpleserver >> - improve output on startup >> - ... and 6 more: >> https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131 > > src/jdk.httpserver/share/classes/sun/net/httpserver/simpleserver/SimpleFileServerImpl.java > line 135: > >> 133: var socketAddr = new InetSocketAddress(addr, port); >> 134: var server = SimpleFileServer.createFileServer(socketAddr, >> root, outputLevel); >> 135: server.setExecutor(Executors.newSingleThreadExecutor()); > > I think this code has the effect of creating one thread for the selector and > a second one for the execution of the handlers. If we want to keep thread > usage to an absolute minimum then it might be better to not set an executor. > Then, the selector thread executes the handlers as well. I did a quick stress test of both versions (with and without executor specified) and don’t see any difference in how many requests can be handled. Probably good to keep thread usage to a minimum, I'll update that. - PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
On Tue, 21 Sep 2021 16:04:21 GMT, Daniel Fuchs wrote:
>> Julia Boes has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 16 commits:
>>
>> - Merge branch 'master' into simpleserver
>> - Merge remote-tracking branch 'origin/simpleserver' into simpleserver
>> - Merge branch 'master' into simpleserver
>> - refactor isHidden,isReadable,isSymlink checks and cleanup tests
>> - Merge branch 'master' into simpleserver
>> - check isHidden, isSymlink, isReadable for all path segments
>> - add checks for all path segments
>> - Merge branch 'master' into componentcheck
>> - Merge branch 'master' into simpleserver
>> - improve output on startup
>> - ... and 6 more:
>> https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131
>
> src/jdk.httpserver/share/classes/sun/net/httpserver/simpleserver/FileServerHandler.java
> line 314:
>
>> 312: + "\n");
>> 313: try (var paths = Files.list(path)) {
>> 314: paths.filter(p -> !isHiddenOrSymLink(p))
>
> Shouldn't we filter paths that are not readable here too? There's no point in
> printing a link that will result in 404, is there?
Totally, thanks for noting!
-
PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
On Tue, 21 Sep 2021 15:23:33 GMT, Michael McMahon wrote:
>> Julia Boes has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 16 commits:
>>
>> - Merge branch 'master' into simpleserver
>> - Merge remote-tracking branch 'origin/simpleserver' into simpleserver
>> - Merge branch 'master' into simpleserver
>> - refactor isHidden,isReadable,isSymlink checks and cleanup tests
>> - Merge branch 'master' into simpleserver
>> - check isHidden, isSymlink, isReadable for all path segments
>> - add checks for all path segments
>> - Merge branch 'master' into componentcheck
>> - Merge branch 'master' into simpleserver
>> - improve output on startup
>> - ... and 6 more:
>> https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131
>
> test/jdk/com/sun/net/httpserver/FilterTest.java line 330:
>
>> 328: var response = client.send(request,
>> HttpResponse.BodyHandlers.ofString());
>> 329: assertEquals(response.statusCode(), 200);
>> 330: assertEquals(inspectedURI.get(), URI.create("/"));
>
> Could you make the request URI something like /foo/bar instead of just / here?
Yes, I'll do that.
-
PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
On Tue, 21 Sep 2021 14:09:54 GMT, Julia Boes wrote: >> This change implements a simple web server that can be run on the >> command-line with `java -m jdk.httpserver`. >> >> This is facilitated by adding an entry point for the `jdk.httpserver` >> module, an implementation class whose main method is run when the above >> command is executed. This is the first such module entry point in the JDK. >> >> The server is a minimal HTTP server that serves the static files of a given >> directory, similar to existing alternatives on other platforms and >> convenient for testing, development, and debugging. >> >> Additionally, a small API is introduced for programmatic creation and >> customization. >> >> Testing: tier1-3. > > Julia Boes has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 16 commits: > > - Merge branch 'master' into simpleserver > - Merge remote-tracking branch 'origin/simpleserver' into simpleserver > - Merge branch 'master' into simpleserver > - refactor isHidden,isReadable,isSymlink checks and cleanup tests > - Merge branch 'master' into simpleserver > - check isHidden, isSymlink, isReadable for all path segments > - add checks for all path segments > - Merge branch 'master' into componentcheck > - Merge branch 'master' into simpleserver > - improve output on startup > - ... and 6 more: > https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131 Overall, this is a very nice addition. Good work. - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
On Tue, 21 Sep 2021 14:09:54 GMT, Julia Boes wrote: >> This change implements a simple web server that can be run on the >> command-line with `java -m jdk.httpserver`. >> >> This is facilitated by adding an entry point for the `jdk.httpserver` >> module, an implementation class whose main method is run when the above >> command is executed. This is the first such module entry point in the JDK. >> >> The server is a minimal HTTP server that serves the static files of a given >> directory, similar to existing alternatives on other platforms and >> convenient for testing, development, and debugging. >> >> Additionally, a small API is introduced for programmatic creation and >> customization. >> >> Testing: tier1-3. > > Julia Boes has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 16 commits: > > - Merge branch 'master' into simpleserver > - Merge remote-tracking branch 'origin/simpleserver' into simpleserver > - Merge branch 'master' into simpleserver > - refactor isHidden,isReadable,isSymlink checks and cleanup tests > - Merge branch 'master' into simpleserver > - check isHidden, isSymlink, isReadable for all path segments > - add checks for all path segments > - Merge branch 'master' into componentcheck > - Merge branch 'master' into simpleserver > - improve output on startup > - ... and 6 more: > https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131 src/jdk.httpserver/share/classes/sun/net/httpserver/simpleserver/SimpleFileServerImpl.java line 135: > 133: var socketAddr = new InetSocketAddress(addr, port); > 134: var server = SimpleFileServer.createFileServer(socketAddr, > root, outputLevel); > 135: server.setExecutor(Executors.newSingleThreadExecutor()); I think this code has the effect of creating one thread for the selector and a second one for the execution of the handlers. If we want to keep thread usage to an absolute minimum then it might be better to not set an executor. Then, the selector thread executes the handlers as well. - PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
On Tue, 21 Sep 2021 14:09:54 GMT, Julia Boes wrote: >> This change implements a simple web server that can be run on the >> command-line with `java -m jdk.httpserver`. >> >> This is facilitated by adding an entry point for the `jdk.httpserver` >> module, an implementation class whose main method is run when the above >> command is executed. This is the first such module entry point in the JDK. >> >> The server is a minimal HTTP server that serves the static files of a given >> directory, similar to existing alternatives on other platforms and >> convenient for testing, development, and debugging. >> >> Additionally, a small API is introduced for programmatic creation and >> customization. >> >> Testing: tier1-3. > > Julia Boes has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 16 commits: > > - Merge branch 'master' into simpleserver > - Merge remote-tracking branch 'origin/simpleserver' into simpleserver > - Merge branch 'master' into simpleserver > - refactor isHidden,isReadable,isSymlink checks and cleanup tests > - Merge branch 'master' into simpleserver > - check isHidden, isSymlink, isReadable for all path segments > - add checks for all path segments > - Merge branch 'master' into componentcheck > - Merge branch 'master' into simpleserver > - improve output on startup > - ... and 6 more: > https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131 Changes requested by michaelm (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
On Tue, 21 Sep 2021 16:18:54 GMT, Daniel Fuchs wrote: >> The problem I was referring to was not about printing to the console. I >> hadn't thought about that, I agree the default locale should be used there. >> I was referring to `Last-modified` HTTP headers with a non-English date >> value, e.g.: >> >> Last-modified: Di., 21 Sep. 2021 09:56:53 GMT >> >> I think browsers will get confused by this. It probably isn't a big deal >> since I think the server doesn't implement conditional GETs (i.e. use the >> last-modified date in subsequent requests), but the HTTP spec is [quite >> strict ][1] about date formats. >> >> [1]: https://datatracker.ietf.org/doc/html/rfc2616#section-3.3.1 > > Thanks for clarifying. Yes the date should be in GMT and should not be > localized when sent in a `Last-Modified` / `Date` headers. Good catch! Good catch indeed, thanks! I'll update to GMT when used in the headers. - PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
On Tue, 21 Sep 2021 16:03:28 GMT, Hannes Wallnöfer wrote: >> Hmm... When printing messages on the console don't we want a localized date? >> Or are you referring to other usages of the date formatter? Worth double >> checking in any case. > > The problem I was referring to was not about printing to the console. I > hadn't thought about that, I agree the default locale should be used there. I > was referring to `Last-modified` HTTP headers with a non-English date value, > e.g.: > > Last-modified: Di., 21 Sep. 2021 09:56:53 GMT > > I think browsers will get confused by this. It probably isn't a big deal > since I think the server doesn't implement conditional GETs (i.e. use the > last-modified date in subsequent requests), but the HTTP spec is [quite > strict ][1] about date formats. > > [1]: https://datatracker.ietf.org/doc/html/rfc2616#section-3.3.1 Thanks for clarifying. Yes the date should be in GMT and should not be localized when sent in a `Last-Modified` / `Date` headers. Good catch! - PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
On Tue, 21 Sep 2021 15:27:12 GMT, Daniel Fuchs wrote: >> I just realized I commented on a test file, while the actual culprit is in >> `FileServerHandler.java`. But I guess it applies to all usages of this class >> and method. > > Hmm... When printing messages on the console don't we want a localized date? > Or are you referring to other usages of the date formatter? Worth double > checking in any case. The problem I was referring to was not about printing to the console. I hadn't thought about that, I agree the default locale should be used there. I was referring to `Last-modified` HTTP headers with a non-English date value, e.g.: Last-modified: Di., 21 Sep. 2021 09:56:53 GMT I think browsers will get confused by this. It probably isn't a big deal since I think the server doesn't implement conditional GETs (i.e. use the last-modified date in subsequent requests), but the HTTP spec is [quite strict ][1] about date formats. [1]: https://datatracker.ietf.org/doc/html/rfc2616#section-3.3.1 - PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
On Tue, 21 Sep 2021 14:09:54 GMT, Julia Boes wrote:
>> This change implements a simple web server that can be run on the
>> command-line with `java -m jdk.httpserver`.
>>
>> This is facilitated by adding an entry point for the `jdk.httpserver`
>> module, an implementation class whose main method is run when the above
>> command is executed. This is the first such module entry point in the JDK.
>>
>> The server is a minimal HTTP server that serves the static files of a given
>> directory, similar to existing alternatives on other platforms and
>> convenient for testing, development, and debugging.
>>
>> Additionally, a small API is introduced for programmatic creation and
>> customization.
>>
>> Testing: tier1-3.
>
> Julia Boes has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 16 commits:
>
> - Merge branch 'master' into simpleserver
> - Merge remote-tracking branch 'origin/simpleserver' into simpleserver
> - Merge branch 'master' into simpleserver
> - refactor isHidden,isReadable,isSymlink checks and cleanup tests
> - Merge branch 'master' into simpleserver
> - check isHidden, isSymlink, isReadable for all path segments
> - add checks for all path segments
> - Merge branch 'master' into componentcheck
> - Merge branch 'master' into simpleserver
> - improve output on startup
> - ... and 6 more:
> https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131
test/jdk/com/sun/net/httpserver/FilterTest.java line 330:
> 328: var response = client.send(request,
> HttpResponse.BodyHandlers.ofString());
> 329: assertEquals(response.statusCode(), 200);
> 330: assertEquals(inspectedURI.get(), URI.create("/"));
Could you make the request URI something like /foo/bar instead of just / here?
-
PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
On Tue, 21 Sep 2021 14:09:54 GMT, Julia Boes wrote:
>> This change implements a simple web server that can be run on the
>> command-line with `java -m jdk.httpserver`.
>>
>> This is facilitated by adding an entry point for the `jdk.httpserver`
>> module, an implementation class whose main method is run when the above
>> command is executed. This is the first such module entry point in the JDK.
>>
>> The server is a minimal HTTP server that serves the static files of a given
>> directory, similar to existing alternatives on other platforms and
>> convenient for testing, development, and debugging.
>>
>> Additionally, a small API is introduced for programmatic creation and
>> customization.
>>
>> Testing: tier1-3.
>
> Julia Boes has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 16 commits:
>
> - Merge branch 'master' into simpleserver
> - Merge remote-tracking branch 'origin/simpleserver' into simpleserver
> - Merge branch 'master' into simpleserver
> - refactor isHidden,isReadable,isSymlink checks and cleanup tests
> - Merge branch 'master' into simpleserver
> - check isHidden, isSymlink, isReadable for all path segments
> - add checks for all path segments
> - Merge branch 'master' into componentcheck
> - Merge branch 'master' into simpleserver
> - improve output on startup
> - ... and 6 more:
> https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131
src/jdk.httpserver/share/classes/sun/net/httpserver/simpleserver/FileServerHandler.java
line 314:
> 312: + "\n");
> 313: try (var paths = Files.list(path)) {
> 314: paths.filter(p -> !isHiddenOrSymLink(p))
Shouldn't we filter paths that are not readable here too? There's no point in
printing a link that will result in 404, is there?
-
PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
On Tue, 21 Sep 2021 15:06:09 GMT, Hannes Wallnöfer wrote:
>> test/jdk/com/sun/net/httpserver/simpleserver/SecurityManagerTest.java line
>> 198:
>>
>>> 196:
>>> 197: static final DateTimeFormatter HTTP_DATE_FORMATTER =
>>> 198: DateTimeFormatter.ofPattern("EEE, dd MMM HH:mm:ss v");
>>
>> I think this and other usages of `DateTimeFormatter.ofPattern` need to be
>> called with `Locale.US` (or something similar) as second argument, otherwise
>> the current default locale will be used. I noticed because on my laptop the
>> `Last-modified` header contains a german date.
>
> I just realized I commented on a test file, while the actual culprit is in
> `FileServerHandler.java`. But I guess it applies to all usages of this class
> and method.
Hmm... When printing messages on the console don't we want a localized date? Or
are you referring to other usages of the date formatter? Worth double checking
in any case.
-
PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
On Tue, 21 Sep 2021 14:59:18 GMT, Hannes Wallnöfer wrote:
>> Julia Boes has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 16 commits:
>>
>> - Merge branch 'master' into simpleserver
>> - Merge remote-tracking branch 'origin/simpleserver' into simpleserver
>> - Merge branch 'master' into simpleserver
>> - refactor isHidden,isReadable,isSymlink checks and cleanup tests
>> - Merge branch 'master' into simpleserver
>> - check isHidden, isSymlink, isReadable for all path segments
>> - add checks for all path segments
>> - Merge branch 'master' into componentcheck
>> - Merge branch 'master' into simpleserver
>> - improve output on startup
>> - ... and 6 more:
>> https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131
>
> test/jdk/com/sun/net/httpserver/simpleserver/SecurityManagerTest.java line
> 198:
>
>> 196:
>> 197: static final DateTimeFormatter HTTP_DATE_FORMATTER =
>> 198: DateTimeFormatter.ofPattern("EEE, dd MMM HH:mm:ss v");
>
> I think this and other usages of `DateTimeFormatter.ofPattern` need to be
> called with `Locale.US` (or something similar) as second argument, otherwise
> the current default locale will be used. I noticed because on my laptop the
> `Last-modified` header contains a german date.
I just realized I commented on a test file, while the actual culprit is in
`FileServerHandler.java`. But I guess it applies to all usages of this class
and method.
-
PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
On Tue, 21 Sep 2021 14:09:54 GMT, Julia Boes wrote:
>> This change implements a simple web server that can be run on the
>> command-line with `java -m jdk.httpserver`.
>>
>> This is facilitated by adding an entry point for the `jdk.httpserver`
>> module, an implementation class whose main method is run when the above
>> command is executed. This is the first such module entry point in the JDK.
>>
>> The server is a minimal HTTP server that serves the static files of a given
>> directory, similar to existing alternatives on other platforms and
>> convenient for testing, development, and debugging.
>>
>> Additionally, a small API is introduced for programmatic creation and
>> customization.
>>
>> Testing: tier1-3.
>
> Julia Boes has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 16 commits:
>
> - Merge branch 'master' into simpleserver
> - Merge remote-tracking branch 'origin/simpleserver' into simpleserver
> - Merge branch 'master' into simpleserver
> - refactor isHidden,isReadable,isSymlink checks and cleanup tests
> - Merge branch 'master' into simpleserver
> - check isHidden, isSymlink, isReadable for all path segments
> - add checks for all path segments
> - Merge branch 'master' into componentcheck
> - Merge branch 'master' into simpleserver
> - improve output on startup
> - ... and 6 more:
> https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131
test/jdk/com/sun/net/httpserver/simpleserver/SecurityManagerTest.java line 198:
> 196:
> 197: static final DateTimeFormatter HTTP_DATE_FORMATTER =
> 198: DateTimeFormatter.ofPattern("EEE, dd MMM HH:mm:ss v");
I think this and other usages of `DateTimeFormatter.ofPattern` need to be
called with `Locale.US` (or something similar) as second argument, otherwise
the current default locale will be used. I noticed because on my laptop the
`Last-modified` header contains a german date.
-
PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]
> This change implements a simple web server that can be run on the > command-line with `java -m jdk.httpserver`. > > This is facilitated by adding an entry point for the `jdk.httpserver` module, > an implementation class whose main method is run when the above command is > executed. This is the first such module entry point in the JDK. > > The server is a minimal HTTP server that serves the static files of a given > directory, similar to existing alternatives on other platforms and convenient > for testing, development, and debugging. > > Additionally, a small API is introduced for programmatic creation and > customization. > > Testing: tier1-3. Julia Boes has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 16 commits: - Merge branch 'master' into simpleserver - Merge remote-tracking branch 'origin/simpleserver' into simpleserver - Merge branch 'master' into simpleserver - refactor isHidden,isReadable,isSymlink checks and cleanup tests - Merge branch 'master' into simpleserver - check isHidden, isSymlink, isReadable for all path segments - add checks for all path segments - Merge branch 'master' into componentcheck - Merge branch 'master' into simpleserver - improve output on startup - ... and 6 more: https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131 - Changes: https://git.openjdk.java.net/jdk/pull/5505/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5505&range=04 Stats: 7162 lines in 42 files changed: 7127 ins; 15 del; 20 mod Patch: https://git.openjdk.java.net/jdk/pull/5505.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5505/head:pull/5505 PR: https://git.openjdk.java.net/jdk/pull/5505
