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.

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

Rename 'GRPCClient.run()' #2156

Merged
merged 5 commits into from
Jan 17, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
17 changes: 11 additions & 6 deletions Sources/GRPCCore/GRPCClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,10 @@ private import Synchronization
/// ## Creating a client manually
///
/// If the `with`-style methods for creating clients isn't suitable for your application then you
/// can create and run a client manually. This requires you to call the ``run()`` method in a task
/// can create and run a client manually. This requires you to call the ``maintainConnections()`` method in a task
/// which instructs the client to start connecting to the server.
///
/// The ``run()`` method won't return until the client has finished handling all requests. You can
/// The ``maintainConnections()`` method won't return until the client has finished handling all requests. You can
/// signal to the client that it should stop creating new request streams by calling ``beginGracefulShutdown()``.
/// This gives the client enough time to drain any requests already in flight. To stop the client
/// more abruptly you can cancel the task running your client. If your application requires
Expand Down Expand Up @@ -114,7 +114,7 @@ public final class GRPCClient: Sendable {
func checkExecutable() throws {
switch self {
case .notStarted, .running:
// Allow .notStarted as making a request can race with 'run()'. Transports should tolerate
// Allow .notStarted as making a request can race with 'maintainConnections()'. Transports should tolerate
// queuing the request if not yet started.
()
case .stopping, .stopped:
Expand Down Expand Up @@ -208,7 +208,7 @@ public final class GRPCClient: Sendable {
///
/// The client, and by extension this function, can only be run once. If the client is already
/// running or has already been closed then a ``RuntimeError`` is thrown.
public func run() async throws {
public func maintainConnections() async throws {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor:
I (think I) understand why maintainConnection instead of connect - because it's async thus a long-running task and the act of just connecting doesn't imply it runs for a long period of time. But I find the naming still a bit weird. Did you consider something like connectAndRun()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's fair, I wasn't 100% happy with the name.

Broadly speaking this function is:

  • long running
  • meant to be run in the background
  • will manage any underlying connections

I think maintainConnections does an okay job at capturing that. Some alternatives:

  • connectAndRun
  • runBackgroundWork
  • runBackgroundTasks
  • manageConnections
  • runConnections

WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think runConnections, manageConnections and (I'll add a third one) startAndRunConnections achieve the three goals you listed the best.

try self.stateMachine.withLock { try $0.state.run() }

// When this function exits the client must have stopped.
Expand All @@ -227,6 +227,11 @@ public final class GRPCClient: Sendable {
}
}

@available(*, deprecated, renamed: "maintainConnections", message: "It'll be removed before v2.")
public func run() async throws {
try await self.maintainConnections()
}

/// Close the client.
///
/// The transport will be closed: this means that it will be given enough time to wait for
Expand Down Expand Up @@ -338,7 +343,7 @@ public final class GRPCClient: Sendable {

/// Start a bidirectional streaming RPC.
///
/// - Note: ``run()`` must have been called and still executing, and ``beginGracefulShutdown()`` mustn't
/// - Note: ``maintainConnections()`` must have been called and still executing, and ``beginGracefulShutdown()`` mustn't
/// have been called.
///
/// - Parameters:
Expand Down Expand Up @@ -430,7 +435,7 @@ public func withGRPCClient<Result: Sendable>(
try await withThrowingDiscardingTaskGroup { group in
let client = GRPCClient(transport: transport, interceptorPipeline: interceptorPipeline)
group.addTask {
try await client.run()
try await client.maintainConnections()
}

let result = try await handleClient(client)
Expand Down
12 changes: 6 additions & 6 deletions Tests/GRPCCoreTests/GRPCClientTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ final class GRPCClientTests: XCTestCase {
}

group.addTask {
try await client.run()
try await client.maintainConnections()
}

// Wait for client and server to be running.
Expand Down Expand Up @@ -377,13 +377,13 @@ final class GRPCClientTests: XCTestCase {
let inProcess = InProcessTransport()
let client = GRPCClient(transport: inProcess.client)
// Run the client.
let task = Task { try await client.run() }
let task = Task { try await client.maintainConnections() }
task.cancel()
try await task.value

// Client is stopped, should throw an error.
await XCTAssertThrowsErrorAsync(ofType: RuntimeError.self) {
try await client.run()
try await client.maintainConnections()
} errorHandler: { error in
XCTAssertEqual(error.code, .clientIsStopped)
}
Expand All @@ -393,13 +393,13 @@ final class GRPCClientTests: XCTestCase {
let inProcess = InProcessTransport()
let client = GRPCClient(transport: inProcess.client)
// Run the client.
let task = Task { try await client.run() }
let task = Task { try await client.maintainConnections() }
// Make sure the client is run for the first time here.
try await Task.sleep(for: .milliseconds(10))

// Client is already running, should throw an error.
await XCTAssertThrowsErrorAsync(ofType: RuntimeError.self) {
try await client.run()
try await client.maintainConnections()
} errorHandler: { error in
XCTAssertEqual(error.code, .clientIsAlreadyRunning)
}
Expand Down Expand Up @@ -551,7 +551,7 @@ struct ClientTests {
}

group.addTask {
try await client.run()
try await client.maintainConnections()
}

// Make sure both server and client are running
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ struct InProcessTransportTests {

let client = GRPCClient(transport: inProcess.client)
group.addTask {
try await client.run()
try await client.maintainConnections()
}

try await execute(server, client)
Expand All @@ -60,7 +60,7 @@ struct InProcessTransportTests {
#expect(messages == ["isCancelled=true"])
}

// Finally, shutdown the client so its run() method returns.
// Finally, shutdown the client so its maintainConnections() method returns.
client.beginGracefulShutdown()
}
}
Expand Down
Loading