Okhttp Websocket client 'closeReason' is not completed when websocket fails

[varahash]

Ktor Version and Engine Used (client or server and name)
io.ktor:ktor-client-okhttp:1.2.4

Describe the bug
The 'closeReason' at line https://github.com/ktorio/ktor/blob/1.2.4/ktor-client/ktor-client-okhttp/jvm/src/io/ktor/client/engine/okhttp/OkHttpWebsocketSession.kt#L48
must be completed with 'null' value when websocket fails here: https://github.com/ktorio/ktor/blob/1.2.4/ktor-client/ktor-client-okhttp/jvm/src/io/ktor/client/engine/okhttp/OkHttpWebsocketSession.kt#L93

[cy6erGn0m]

It should complete exceptionally, null makes no sense

[varahash]

Then you should update doc as well:

It could be null if a session is terminated with no close reason (for example due to connection failure).

https://github.com/ktorio/ktor/blob/1.2.4/ktor-http/ktor-http-cio/common/src/io/ktor/http/cio/websocket/DefaultWebSocketSession.kt#L24-L26

[dmitrievanthony]

Hi @varahash, @cy6erGn0m,

the only issue I found is that OkHttp actually does not complete close reason in case of failure. I fixed it in the PR above.

[varahash]

Why documentation says that closeReason can completes with null and as example provides connection failure?

ktor/ktor-http/ktor-http-cio/common/src/io/ktor/http/cio/websocket/DefaultWebSocketSession.kt

Lines 23 to 27 in 08fcd12

	/**
	* A close reason for this session. It could be `null` if a session is terminated with no close reason
	* (for example due to connection failure).
	*/
	val closeReason: Deferred<CloseReason?>

How CIO implementation completes closeReason, exceptionally or with null?
If exceptionally, what is real case for closeReason to be null?

[dmitrievanthony]

@varahash, the documentation says so because in some corner cases closeReason could be null and users should take this into account. As far as I remember, particularly CIO completes it with null.

[varahash]

If so, maybe it is better to make OkHttp version match CIO implementation.

[dmitrievanthony]

Well, if we are talking from a design perspective I think it would be more informative to pass exception in CIO.

[dmitrievanthony]

I merged the corresponding PR, so the fix is in master now. Feel free to reopen the issue if you have additional comments.