square / okhttp

Square’s meticulous HTTP client for the JVM, Android, and GraalVM.
https://square.github.io/okhttp/
Apache License 2.0
45.76k stars 9.15k forks source link

RealWebSocket.close() throws IllegalArgumentException when code is 0 #3531

Closed Egorand closed 7 years ago

Egorand commented 7 years ago

In the Javadoc for WebSocket.close():

@param code Status code as defined by <a href="...">Section 7.4 of RFC 6455</a> or {@code 0}.

When doing the following:

WebSocket webSocket = okHttpClient.newWebSocket(request, wsListener);
webSocket.close(0, "Bye!");

It throws:

java.lang.IllegalArgumentException: Code must be in range [1000,5000): 0

    at okhttp3.internal.ws.WebSocketProtocol.validateCloseCode(WebSocketProtocol.java:120)
    at okhttp3.internal.ws.RealWebSocket.close(RealWebSocket.java:399)
    at okhttp3.internal.ws.RealWebSocket.close(RealWebSocket.java:395)

It's not clear what 0 would mean anyway, the RFC states that this code is not used. Does it make sense to just fix the Javadoc?

JakeWharton commented 7 years ago

It may have been changed after running the autobahn test suite. Doss the commit history offer any answers? I can look later otherwise.

On Sat, Aug 19, 2017, 5:24 PM Egor Andreevici notifications@github.com wrote:

In the Javadoc for WebSocket.close():

@param code Status code as defined by Section 7.4 of RFC 6455 or {@code 0}.

When called with the following args:

webSocket.close(0, "Bye!");

It throws:

java.lang.IllegalArgumentException: Code must be in range [1000,5000): 0

at okhttp3.internal.ws.WebSocketProtocol.validateCloseCode(WebSocketProtocol.java:120) at okhttp3.internal.ws.RealWebSocket.close(RealWebSocket.java:399) at okhttp3.internal.ws.RealWebSocket.close(RealWebSocket.java:395)

It's not clear what 0 would mean anyway, the RFC https://tools.ietf.org/html/rfc6455#section-7.4.2 states that this code is not used. Does it make sense to just fix the Javadoc?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/square/okhttp/issues/3531, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEES8B5GY_HQxwsL0rE3aRqZKrCMoCks5sZ1KagaJpZM4O8clm .

Egorand commented 7 years ago

I found c8dbcce that adds validateCloseCode(code) to close()

yschimke commented 7 years ago

WebSocketWriter handles this and tests expect it

if (code != 0) {
        validateCloseCode(code);
}

e.g. https://github.com/square/okhttp/blob/802c029b82672ea90534e8da1bd42100c194aca9/okhttp-tests/src/test/java/okhttp3/internal/ws/WebSocketWriterTest.java#L271

  @Test public void clientEmptyClose() throws IOException {
    clientWriter.writeClose(0, null);
    assertData("888060b420bb");
  }
yschimke commented 7 years ago

I think we agreed to just change the docs https://github.com/square/okhttp/pull/3540