-
Notifications
You must be signed in to change notification settings - Fork 9
Implement Structured MemcachedError for Enhanced Error Handling #33
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
Changes from 15 commits
764e3f7
882b5a0
f504be3
b124153
0bb4f8b
eac8f0a
1e79a89
c486acc
f863db2
153b018
ea8a16a
b1174b1
bcc69b7
bf23544
e6bd8dc
fe93dfa
ae2e9ed
545e643
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -53,18 +53,6 @@ public actor MemcachedConnection { | |||||||||
case finished | ||||||||||
} | ||||||||||
|
||||||||||
/// Enum representing the possible errors that can be encountered in `MemcachedConnection`. | ||||||||||
enum MemcachedConnectionError: Error { | ||||||||||
/// Indicates that the connection has shut down. | ||||||||||
case connectionShutdown | ||||||||||
/// Indicates that a nil response was received from the server. | ||||||||||
case unexpectedNilResponse | ||||||||||
/// Indicates that the key was not found. | ||||||||||
case keyNotFound | ||||||||||
/// Indicates that the key already exist | ||||||||||
case keyExist | ||||||||||
} | ||||||||||
|
||||||||||
private var state: State | ||||||||||
|
||||||||||
/// Initialize a new MemcachedConnection. | ||||||||||
|
@@ -92,7 +80,12 @@ public actor MemcachedConnection { | |||||||||
/// to the server is finished or the task that called this method is cancelled. | ||||||||||
public func run() async throws { | ||||||||||
guard case .initial(let eventLoopGroup, let bufferAllocator, let stream, let continuation) = state else { | ||||||||||
throw MemcachedConnectionError.connectionShutdown | ||||||||||
throw MemcachedError( | ||||||||||
code: .connectionShutdown, | ||||||||||
message: "The connection to the Memcached server has shut down.", | ||||||||||
cause: nil, | ||||||||||
location: MemcachedError.SourceLocation.here() | ||||||||||
) | ||||||||||
} | ||||||||||
|
||||||||||
let channel = try await ClientBootstrap(group: eventLoopGroup) | ||||||||||
|
@@ -128,7 +121,12 @@ public actor MemcachedConnection { | |||||||||
case .running: | ||||||||||
self.state = .finished | ||||||||||
requestContinuation.finish() | ||||||||||
continuation.resume(throwing: error) | ||||||||||
continuation.resume(throwing: MemcachedError( | ||||||||||
code: .connectionShutdown, | ||||||||||
message: "The connection to the Memcached server has shut down while processing a request.", | ||||||||||
cause: error, | ||||||||||
location: MemcachedError.SourceLocation.here() | ||||||||||
)) | ||||||||||
case .initial, .finished: | ||||||||||
break | ||||||||||
} | ||||||||||
|
@@ -151,14 +149,24 @@ public actor MemcachedConnection { | |||||||||
case .enqueued: | ||||||||||
break | ||||||||||
case .dropped, .terminated: | ||||||||||
continuation.resume(throwing: MemcachedConnectionError.connectionShutdown) | ||||||||||
continuation.resume(throwing: MemcachedError( | ||||||||||
code: .connectionShutdown, | ||||||||||
message: "Unable to enqueue request due to the connection being dropped or terminated.", | ||||||||||
|
message: "Unable to enqueue request due to the connection being dropped or terminated.", | |
message: "Unable to enqueue request due to the connection being shutdown.", |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message: "The connection to the Memcached server has shut down.", | |
message: "The connection to the Memcached server has been shut down.", |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for all of these methods e.g. get
, set
etc. We don't have to do a state check, we can just call self.sendRequest
which is doing the check anyhow. That should make our implementation here a lot shorter.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarifying my other comment here. Instead of throwing unexpectedNilResponse
let's name this error protocolError
instead.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message: "Received an unexpected nil response from the Memcached server.", | |
message: "Received an unexpected nil value for a get request.", |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good to document!
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code: .unexpectedNilResponse, | |
message: "Received an unexpected nil response from the Memcached server.", | |
code: .protocolError, | |
message: "Received an unexpected return code \(response.returnCode) for a delete request.", |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also good to document!
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above please with the unexpected return code
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: (Everywhere please)
location: MemcachedError.SourceLocation.here() | |
location: .here() |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just put that comment everywhere
/// - Throws: A `MemcachedError` with the code `.connectionShutdown` if the connection to the Memcache server is shut down. | |
/// - Throws: A `MemcachedError` that indicates the failure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.