aws / aws/aws-lambda-rust-runtime

Fix RuntimeApiClientFuture Visibility and Response Handling

Đang mở
#1,110 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
bug
Ngôn ngữ chính
Rust
Star
3.6k
Fork
396
Chỉ số merge pull request
Không có pull request nào được merge trong 30 ngày

Mô tả

## Background

The `RuntimeApiClientFuture` enum in `lambda-runtime/src/layers/api_client.rs` has several design issues that prevent proper error handling and violate Rust API design best practices. These issues were identified during discussions in PRs [#1091](https://github.com/aws/aws-lambda-rust-runtime/pull/1091) and [#1105](https://github.com/aws/aws-lambda-rust-runtime/pull/1105).

## Current Problems

### 1. Missing `#[non_exhaustive]` Attribute

The `RuntimeApiClientFuture` enum is publicly exposed but lacks the `#[non_exhaustive]` attribute:

```rust
pub enum RuntimeApiClientFuture {
First(#[pin] F, Arc),
Second(#[pin] BoxFuture<'static, Result, BoxError>>),
}
```

This is a semver hazard because:
- Adding new variants becomes a breaking change for downstream users who pattern match on this enum
- The enum is accessible as an associated type on `RuntimeApiClientService`, which is publicly visible
- Any user building custom runtimes on top of our runtime client could be affected

### 2. Incorrect Visibility

The enum and its variants are fully public, exposing internal implementation details that were likely not intended to be part of the public API. As noted in PR #1105:

### 3. Cannot Access Response Body

The current implementation discards the response body for non-2xx responses, making it impossible to read error details from the Lambda Runtime API:

```rust
Ok(resp) if !resp.status().is_success() => {
let status = resp.status();

log_or_print!(
tracing: tracing::error!(status = %status, "Lambda Runtime API returned non-200 response"),
fallback: eprintln!("Lambda Runtime API returned non-200 response: status={status}")
);

// We cannot access resp.body() here to log the actual error message!

break Ok(()); // Returns Ok despite error!
}
```

This is particularly problematic for 410 Gone responses (function timeouts), where we have to add hardcoded messages instead of reading the actual error from the API.

### 4. Returns `Ok(())` for Non-2xx Status Codes

The most counterintuitive behavior is that the client returns `Ok(())` even when the Lambda Runtime API returns error status codes:

```rust
Ok(resp) if !resp.status().is_success() => {
// ... logging ...

// Return Ok to maintain existing contract - runtime continues despite API errors
break Ok(());
}
```

This makes error handling confusing. A non-2xx HTTP response should be treated as an error, not success.

## Impact Assessment

Based on code search analysis mentioned in PR #1105:

The impact is expected to be minimal:
- Internal machinery doesn't rely on the specific type surface
- Only users who built custom higher-level runtimes would be affected
- Forks of the api client would need updates

## Proposed Solutions

Since these changes are breaking, we should bundle them together in a single major version bump. Here are the recommended approaches:

### Option 1: Add `#[non_exhaustive]` and Fix Response Handling (Recommended)

**Changes:**
1. Add `#[non_exhaustive]` to `RuntimeApiClientFuture`
2. Change the return type to properly represent errors
3. Read and log response bodies for non-2xx responses
4. Return `Err` for non-2xx status codes instead of `Ok`

**Implementation:**
```rust
#[non_exhaustive]
pub enum RuntimeApiClientFuture {
First(#[pin] F, Arc),
Second(#[pin] BoxFuture<'static, Result, BoxError>>),
}

impl Future for RuntimeApiClientFuture
where
F: Future, BoxError>>,
{
type Output = Result<(), BoxError>;

fn poll(mut self: Pin<&mut Self>, cx: &mut task::Context<'_>) -> task::Poll {
task::Poll::Ready(loop {
match self.as_mut().project() {
// ... First variant handling ...
RuntimeApiClientFutureProj::Second(fut) => match ready!(fut.poll(cx)) {
Ok(resp) if !resp.status().is_success() => {
let status = resp.status();

// Read the response body to get actual error details
let body = read_body_to_string(resp).await;

log_or_print!(
tracing: tracing::error!(
status = %status,
body = %body,
"Lambda Runtime API returned non-2xx response"
),
fallback: eprintln!(
"Lambda Runtime API returned non-2xx response: status={status}, body={body}"
)
);

// Return Err for non-2xx responses
break Err(format!("Runtime API error: {} - {}", status, body).into());
}
Ok(_) => break Ok(()),
Err(err) => break Err(err),
},
}
})
}
}
```

**Pros:**
- Fixes all issues in one go
- Proper error semantics
- Better debugging experience with actual error messages
- Future-proof with `#[non_exhaustive]`

**Cons:**
- Breaking change for anyone matching on the enum
- Breaking change for anyone relying on `Ok(())` for non-2xx responses

### Option 2: Deprecate and Replace with New Type

**Changes:**
### Option 2: Deprecate and Replace with New Type

**Changes:**
1. Create a new `RuntimeApiClientFutureV2` with correct attributes
2. Create a new `RuntimeApiClientServiceV2` that uses the new future type
3. Deprecate the old `RuntimeApiClientFuture` and `RuntimeApiClientService`
4. Update internal usage to new types
5. Remove old types in next major version

**Rationale:**
Since `RuntimeApiClientFuture` is exposed as an associated type on `RuntimeApiClientService`, we cannot change the future type without also creating a new service type. The service's `Future` associated type is part of its public API contract.

**Implementation:**
```rust
// Old types - deprecated
#[deprecated(since = "0.x.0", note = "Use RuntimeApiClientServiceV2 instead")]
pub struct RuntimeApiClientService {
inner: S,
client: Arc,
}

#[deprecated(since = "0.x.0", note = "Use RuntimeApiClientFutureV2 instead")]
pub enum RuntimeApiClientFuture {
First(#[pin] F, Arc),
Second(#[pin] BoxFuture<'static, Result, BoxError>>),
}

// New types - correct design
pub struct RuntimeApiClientServiceV2 {
inner: S,
client: Arc,
}

impl Service for RuntimeApiClientServiceV2
where
S: Service,
S::Future: Future, BoxError>>,
S::Error: Into,
{
type Response = ();
type Error = BoxError;
type Future = RuntimeApiClientFutureV2;

// ... implementation ...
}

#[non_exhaustive]
pub enum RuntimeApiClientFutureV2 {
First(#[pin] F, Arc),
Second(#[pin] BoxFuture<'static, Result, BoxError>>),
}
```

**Pros:**
- Gentler migration path
- Gives users time to update
- Old behavior remains available during deprecation period

**Cons:**
- Maintains broken behavior longer
- Significantly more code to maintain during transition (duplicate service + future implementations)
- Still requires breaking change eventually
- Users need to update both service and future type references
- More complex migration story

### Option 3 Changing the overall behaviour of client returning body.

Another solution proposed by @jlizen is to directly change how the client works at in `lambda-rust-api-client`. Like the following.

```rust
self.client
.request(req)
.map_err(Into::into)
.then(|res| async move {
match res {
Ok(resp) if !resp.status().is_success() => {
let status = resp.status();
match resp.into_body().await {
// convert into a BoxError containing the status and optional body
// which is nicely loggable in the upper layer, or you could also log it here.
// If you need to specifically log ONLY this case rather than other BoxErrors, you could newtype
// a marker type that we downcast the stderror to
Err(BoxError::new(format!("my message with status code + body")))
}
}
_ => res,
}
})
.boxed()
```

**Pros:**
- In this case we are only breaking the behaviour of client returning OK instead of ERR. It is a behavioural breaking chane.
- Proper error semantics
- Better debugging experience with actual error messages

**Cons:**
- `RuntimeApiClientFuture` is not fixed
- await at `Client` level.

### Error Types

Consider creating a dedicated error type for Runtime API errors:

```rust
#[derive(Debug, thiserror::Error)]
pub enum RuntimeApiError {
#[error("Runtime API returned {status}: {body}")]
NonSuccessResponse { status: StatusCode, body: String },

#[error("Request build failed: {0}")]
RequestBuildError(#[source] BoxError),

#[error("Network error: {0}")]
NetworkError(#[source] BoxError),
}
```

## References

- PR #1091: https://github.com/aws/aws-lambda-rust-runtime/pull/1091
- PR #1105: https://github.com/aws/aws-lambda-rust-runtime/pull/1105
- PR https://github.com/aws/aws-lambda-rust-runtime/pull/1109
- Current implementation: `lambda-runtime/src/layers/api_client.rs`

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Hướng nghiên cứu

Bắt đầu với lambda-runtime/src/layers/api_client.rs và xem xét các PR #1091, #1105 và #1109 để hiểu các đánh đổi chưa được giải quyết liên quan đến API và việc xử lý response. Công việc hoàn tất khi một cách tiếp cận được thống nhất, vấn đề về visibility và exhaustiveness của RuntimeApiClientFuture được xử lý, và các response không phải 2xx giữ lại body của chúng và trả về các error result.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
aws, rust
Lĩnh vực
api, backend
Loại issue
Lỗi
Độ khó
5/5
Thời gian dự kiến
Hơn một tuần
Mức độ hoạt động
Đình trệ
Độ rõ ràng
Khá rõ ràng
Mức phù hợp với người mới
25/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.