Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add WebSocketMockServer and tests for WebSocketEngine #5187

Merged
merged 18 commits into from
Aug 18, 2023
Merged

Add WebSocketMockServer and tests for WebSocketEngine #5187

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
45b2f76
Add a engine tests module and move a few tests there
BoD Aug 9, 2023
8558810
Revert duplicating http tests in integration-tests
BoD Aug 9, 2023
e9f5632
Add WebSocketServer and WebSocketEngineTest
BoD Aug 10, 2023
5800f39
Move tests to common, and fix a few error handling discrepancies betw…
BoD Aug 11, 2023
fddadf6
Move WebSocketMockServer to apollo-mockserver
BoD Aug 16, 2023
19f61e0
Revert duplicating tests in websockets project
BoD Aug 16, 2023
42c35b9
Update API dump
BoD Aug 16, 2023
26c3508
Fix compilation error
BoD Aug 16, 2023
a7afb52
Make tests pass and add logs
BoD Aug 16, 2023
907aff6
Debugging tests...
BoD Aug 16, 2023
142cef4
Remove flaky check in test
BoD Aug 16, 2023
e30fae3
Use 0 for a random port
BoD Aug 17, 2023
7295310
Improve timeout docs
BoD Aug 17, 2023
0bae9cf
Move commonAppleJvmMain to "concurrent" and make it common
BoD Aug 17, 2023
3283368
stop -> close
BoD Aug 17, 2023
f863ca3
Rename Frame -> Message
BoD Aug 17, 2023
b51a1ec
Use socketTimeoutMillis on Apple/Jvm and requestTimeoutMillis on JS
BoD Aug 17, 2023
4f8e1f1
Fix apiDump
BoD Aug 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ...ngine-ktor/src/commonMain/kotlin/com/apollographql/apollo3/network/http/KtorHttpEngine.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,15 @@ class KtorHttpEngine(

private var disposed = false

/**
* @param timeoutMillis: The timeout in milliseconds used both for the connection and the request.
*/
constructor(timeoutMillis: Long = 60_000) : this(timeoutMillis, timeoutMillis)

/**
* @param connectTimeoutMillis The connection timeout in milliseconds. The connection timeout is the time period in which a client should establish a connection with a server.
* @param requestTimeoutMillis The request timeout in milliseconds. The request timeout is the time period required to process an HTTP call: from sending a request to receiving a response.
*/
constructor(connectTimeoutMillis: Long, requestTimeoutMillis: Long) : this(
HttpClient {
expectSuccess = false
Expand Down
8 changes: 4 additions & 4 deletions ...apollo-runtime/src/commonMain/kotlin/com/apollographql/apollo3/network/http/HttpEngine.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ interface HttpEngine {
/**
* @param timeoutMillis: The timeout interval to use when connecting or waiting for additional data.
*
* - on iOS, it is used to set [NSMutableURLRequest.timeoutIntervalForRequest]
* - on Android, it is used to set both [OkHttpClient.connectTimeout] and [OkHttpClient.readTimeout]
* - on Js, it is used to set both connectTimeoutMillis, and socketTimeoutMillis
* - on iOS (NSURLRequest), it is used to set `NSMutableURLRequest.setTimeoutInterval`
* - on Android (OkHttp), it is used to set both `OkHttpClient.connectTimeout` and `OkHttpClient.readTimeout`
* - on Js (Ktor), it is used to set both `HttpTimeoutCapabilityConfiguration.connectTimeoutMillis` and `HttpTimeoutCapabilityConfiguration.requestTimeoutMillis`
*/
expect class DefaultHttpEngine(timeoutMillis: Long = 60_000): HttpEngine

Expand All @@ -61,4 +61,4 @@ class HttpCall(private val engine: HttpEngine, method: HttpMethod, url: String)
}

suspend fun execute(): HttpResponse = engine.execute(requestBuilder.build())
}
}
8 changes: 7 additions & 1 deletion ...apollo-runtime/src/jsMain/kotlin/com/apollographql/apollo3/network/http/KtorHttpEngine.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,16 @@ import io.ktor.util.flattenEntries
import io.ktor.utils.io.CancellationException
import okio.Buffer


/**
* @param connectTimeoutMillis The connection timeout in milliseconds. The connection timeout is the time period in which a client should establish a connection with a server.
* @param readTimeoutMillis The request timeout in milliseconds. The request timeout is the time period required to process an HTTP call: from sending a request to receiving a response.
*/
actual class DefaultHttpEngine constructor(private val connectTimeoutMillis: Long, private val readTimeoutMillis: Long) : HttpEngine {
Comment on lines +22 to 24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're at it, do you mind renaming readTimeoutMillis to requestTimeoutMillis? Ultimately, I'd like us to support readTimeoutMillis because:

  • I think it's a better parameter because it does not depend on the response size
  • requestTimeout can be emulated quite easily using withTimeout while readTimeoutMillis actually requires low-level configuration

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So all in all it'd be:

  • connectTimeout: the initial TCP handshake timeout
  • readTimeout: the socket timeout on subsequent reads
  • requestTimeout (not very useful): the overall timeout of the request

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think renaming readTimeoutMillis to requestTimeoutMillis is breaking right?

Also, in my tests, readTimeoutMillis didn't seem to work actually 😅 But I'll try again to confirm.

In any case I think it makes sense to add a constructor with the 3 parameters? Give more power to the user.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think renaming readTimeoutMillis to requestTimeoutMillis is breaking right?

Indeed, source breaking, which unfortunately isn't tracked by kbcv.

In any case I think it makes sense to add a constructor with the 3 parameters? Give more power to the user.

I'd vote for only 2: connectTimeout and readTimeout. requestTimeout belongs in higher layers using withTimeout. That is if readTimeout is working as expected of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked and socketTimeout doesn't work on Ktor JS. I'll open an issue. In the meantime we could use socketTimeout for Jvm and Apple, but requestTimeout for JS?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so? If you're talking about the expect/actual constructor that takes a simple timeoutMillis parameter then it's fine to map it to socketTimeout on Jvm/Apple and requestTimeout on JS as long as it's properly documented.

Then for overloaded (non-actual) constructors, my preference would go to connectTimeout and readTImeout and leave requestTimeout out of it.

var disposed = false

/**
* @param timeoutMillis: The timeout in milliseconds used both for the connection and the request.
*/
actual constructor(timeoutMillis: Long) : this(timeoutMillis, timeoutMillis)

private val client = HttpClient(Js) {
Expand Down
Loading