> On 29 Mar 2016, at 20:16, Andrej Golovnin <andrej.golov...@gmail.com> wrote:
>
> JEP-280 does not apply in this case. But when you rewrite this code to
> use the plain String concatenation, then JEP-280 would apply:
>
> 211 static String toString(Buffer b) {
> 212 return toStringSimple(b)
> 214 + "[pos=“ + b.position()
> 215 + " lim=“ + b.limit()
> 216 + " cap=“ + b.capacity()
> 217 + " rem=“ + b.remaining() + "]";
> 218 }
>
> This code is easier to read than the one with StringBuilder, at least for me.
I will rewrite this (and similar) code using "+".
>>> src/java.httpclient/share/classes/java/net/http/SignalHandler.java
>>>
>>> Maybe the lines 33-46 should be converted to a JavaDocs comment.
>>>
>>> 112 switch (s) {
>>> 113 case RUNNING:
>>> 114 return RERUN;
>>> 115 case DONE:
>>> 116 return RUNNING;
>>> 117 case RERUN:
>>> 118 return RERUN;
>>> 119 default:
>>> 120 throw new InternalError(String.valueOf(s));
>>> 121 }
>>>
>>> Please fix the indentation.
>>
>> I've tried to adhere to this:
>>
>> http://cr.openjdk.java.net/~alundblad/styleguide/#toc-indentation
>>
>> Might revisit later. I will wait for the status change of this document.
>
> Btw. have you considered to use an array instead of switch-statement?
> The array may look like this:
>
> private static final int[] NEXT_STATE = new int[] {
> RUNNING, // DONE -> RUNNING
> RERUN, // RUNNING -> RERUN
> RERUN, // RERUN -> RERUN
> };
>
> And then the line 111 can be written as:
>
> 111 int prev = state.getAndUpdate(s -> NEXT_STATE[s]);
>
> The only advantage is that it produces less byte codes.
> But maybe it is still worth.
I don't think it would be as readable as switch-case is.
BTW, it looks like there's a newer (Draft, v6) version of the document
(accessible through the same link), which fixes the switch-case formatting
style. Quite a volatile document, indeed :-)
I will fix the indentation.
> I have already started to port our code to use the new API and I have one
> comment.
> I think you should introduce a new class CloseReason as it was done in JSR 356
> (http://docs.oracle.com/javaee/7/api/javax/websocket/CloseReason.html)
> and use it in #sendClose and #onClose-methods. The #onClose-method in the
> WebSocket.Listener interface looks really ugly:
>
> onClose(WebSocket webSocket, Optional<WebSocket.CloseCode> code, String
> reason)
>
> The parameter “reason” makes only sense when code.isPresent() returns true.
> The code and the reason belong together. But the one is wrapped in Optional
> and the other not.
> It doesn’t feel right. But when you introduce the new class CloseReason, then
> you can change this method to:
>
> onClose(WebSocket webSocket, Optional<WebSocket.CloseReason> closeReason)
>
> I think it makes the API much better and it is clear that the reason phrase
> is only
> available when you have a close code too.
Very true. The API looks awkward in this particular place. On the other hand, it
would look a bit of an overkill for the close type of message to define 2 types
(3 types in JSR 356).
Maybe we could somehow melt what you call CloseReason and CloseCode into a
single entity? Something like (different name is an option too):
public final class CloseReason {
static CloseReason normalClosure(CharSequence reason);
static CloseReason goingAway(CharSequence reason);
// ...
static CloseReason unexpectedCondition(CharSequence reason);
static CloseReason of(int code);
static CloseReason of(int code, CharSequence reason);
}
In an application:
ws.sendClose(CloseReason.normalClosure("Too tired"));
ws.sendClose(CloseReason.of(2016, "Leap year"));
This doesn't solve the first problem of not having code description, but seems
to solve the second one.
> And I think there should be a way to configure a timeout for sending messages.
As I understand timeout expiration for sendXXX would be a fatal condition (in
general.) Simply because by the time a returned CF has completed exceptionally,
some bytes of a message might have been sent already. (That's generally not the
case with read timeouts see java.net.Socket#setSoTimeout)
What would you expect _additionally_ from the implementation (API guarantees) in
a case where send completes exceptionally with a timeout in contrast with this
snippet?
ws.sendBinary(giganticByteBuffer, true)
.orTimeout(30, TimeUnit.SECONDS)
.whenComplete((v, e) -> {
if (e instanceof TimeoutException) {
try {
ws.abort();
} catch (IOException ignored) { }
}
});
BTW, I'm thinking of changing sendXXX methods return type to CompletionStage
(now it's CompletableFuture), this will cause, for example,
CompletableFuture#get(long, java.util.concurrent.TimeUnit) not to be directly
accessible.
> One minor thing off-topic:
> While changing my code I noticed that the class
> ProxySelector$StaticProxySelector
> and the JavaDocs of the ProxySelector#of-method can be improved a little bit:
>
> diff -r 589795e4cd38 src/java.base/share/classes/java/net/ProxySelector.java
> --- a/src/java.base/share/classes/java/net/ProxySelector.java Wed Mar 23
> 19:33:39 2016 -0700
> +++ b/src/java.base/share/classes/java/net/ProxySelector.java Mon Mar 28
> 19:30:24 2016 +0200
> @@ -165,7 +165,8 @@
>
> /**
> * Returns a ProxySelector which uses the given proxy address for all HTTP
> - * and HTTPS requests. If proxy is {@code null} then proxying is
> disabled.
> + * and HTTPS requests. If the given proxy address is {@code null}
> + * then proxying is disabled.
> *
> * @param proxyAddress
> * The address of the proxy
> @@ -178,9 +179,9 @@
> return new StaticProxySelector(proxyAddress);
> }
>
> - static class StaticProxySelector extends ProxySelector {
> + private static final class StaticProxySelector extends ProxySelector {
> private static final List<Proxy> NO_PROXY_LIST =
> List.of(Proxy.NO_PROXY);
> - final List<Proxy> list;
> + private final List<Proxy> list;
>
> StaticProxySelector(InetSocketAddress address){
> Proxy p;
> @@ -198,9 +199,9 @@
> }
>
> @Override
> - public synchronized List<Proxy> select(URI uri) {
> - String scheme = uri.getScheme().toLowerCase();
> - if (scheme.equals("http") || scheme.equals("https")) {
> + public List<Proxy> select(URI uri) {
> + String scheme = uri.getScheme();
> + if (scheme.equalsIgnoreCase("http") ||
> scheme.equalsIgnoreCase("https")) {
> return list;
> } else {
> return NO_PROXY_LIST;
CC'ing Michael.
