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

[RFC] Adopts *Writable* BaggageContext (Better) #167

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 2 additions & 0 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ let package = Package(
dependencies: [
.package(url: "https://github.com/apple/swift-nio.git", .upToNextMajor(from: "2.17.0")),
.package(url: "https://github.com/apple/swift-log.git", .upToNextMajor(from: "1.0.0")),
.package(name: "swift-context", url: "https://github.com/ktoso/gsoc-swift-baggage-context.git", .branch("simple-is-good-proposal")), // TODO: use main once merged
.package(url: "https://github.com/swift-server/swift-backtrace.git", .upToNextMajor(from: "1.1.0")),
],
targets: [
Expand All @@ -27,6 +28,7 @@ let package = Package(
]),
.target(name: "AWSLambdaRuntimeCore", dependencies: [
.product(name: "Logging", package: "swift-log"),
.product(name: "BaggageContext", package: "swift-context"),
.product(name: "Backtrace", package: "swift-backtrace"),
.product(name: "NIOHTTP1", package: "swift-nio"),
]),
Expand Down
184 changes: 158 additions & 26 deletions Sources/AWSLambdaRuntimeCore/LambdaContext.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
//
//===----------------------------------------------------------------------===//

import BaggageContext
import Dispatch
import Logging
import NIO
Expand Down Expand Up @@ -49,40 +50,132 @@ extension Lambda {
extension Lambda {
/// Lambda runtime context.
/// The Lambda runtime generates and passes the `Context` to the Lambda handler as an argument.
public final class Context: CustomDebugStringConvertible {
public struct Context: BaggageContext.Context, CustomDebugStringConvertible {
/// Used to store all contents of the context and implement CoW semantics for it.
private var storage: Storage

final class Storage {
var baggage: Baggage

let invokedFunctionARN: String
let deadline: DispatchWallTime
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deadlines we'll eventually be able to handle in a distributed way -- so this will eventually move into the baggage.

See: Future: Deadlines and Cancelation patterns slashmo/gsoc-swift-baggage-context#24

let cognitoIdentity: String?
let clientContext: String?
ktoso marked this conversation as resolved.
Show resolved Hide resolved

// Implementation note: This logger is the "user provided logger" that we will log to when `context.logger` is used.
// It must be updated with the latest metadata whenever the `baggage` changes.
var _logger: Logger
ktoso marked this conversation as resolved.
Show resolved Hide resolved

let eventLoop: EventLoop
let allocator: ByteBufferAllocator

init(
baggage: Baggage,
invokedFunctionARN: String,
deadline: DispatchWallTime,
cognitoIdentity: String?,
clientContext: String?,
logger: Logger,
eventLoop: EventLoop,
allocator: ByteBufferAllocator
) {
self.baggage = baggage
self.invokedFunctionARN = invokedFunctionARN
self.deadline = deadline
self.cognitoIdentity = cognitoIdentity
self.clientContext = clientContext
self._logger = logger
self.eventLoop = eventLoop
self.allocator = allocator
}
}

/// Contains contextual metadata such as request and trace identifiers, along with other information which may
/// be carried throughout asynchronous and cross-node boundaries (e.g. through HTTPClient calls).
public var baggage: Baggage {
get {
self.storage.baggage
}
set {
if isKnownUniquelyReferenced(&self.storage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please correct me if i am wrong, as far as I understand it:

  1. everything (new storage object) is copied every time baggage is updated, which will happen every time new span is created = often

  2. this does implement copy on write however changing of the storage is not thread safe which I think is not a nice API for user who would need to sync the changes himself

see discussion here https://gist.github.com/drewmccormack/b1c4487935cf3c3e0a5feaf488a95ebd#gistcomment-2826755

Copy link
Contributor Author

@ktoso ktoso Sep 24, 2020

Choose a reason for hiding this comment

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

Yeah I'm aware of that rule, it could be that it's misapplied here after all... I'll dig into it again.

With the latest changes in context perhaps actually we can get away without the Storage completely after all actually. Meh we'd want to keep CoW here to keep the lambda context small. I'll look into this shortly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pokryfka, so no. This is implementation correct. (You had me worried there for a second! 😉)

This is as thread safe as an Int or any other value semantics type is.

isKnownUniquelyReferenced is commonly misunderstood, so I understand where your confusion is coming from. However such implementation is safe by itself. Now how someone uses this type may or may not be thread-safe -- and that is what the example you're linking is doing wrong. The same kind of race can be exhibited on even a plain integer value. This type provides value semantics guarantees, and no more.


everything (new storage object) is copied every time baggage is updated, which will happen every time new span is created = often

I disagree about the "often" quantification used here. It is by far not as often as a raw struct would be copying -- each passing around and reassigning to a new variable even would technically then be copying.

We're getting the best of both worlds; and simply following the known CoW semantics that are frequent in Swift.

this does implement copy on write however changing of the storage is not thread safe which I think is not a nice API for user who would need to sync the changes himself

The operations in this piece of code are safe. If they weren't then Array and all of Swift's collections are screwed as well 😉 We are providing the same semantics as those types.

Copy link
Contributor

Choose a reason for hiding this comment

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

everything (new storage object) is copied every time baggage is updated, which will happen every time new span is created = often

I disagree about the "often" quantification used here. It is by far not as often as a raw struct would be copying -- each passing around and reassigning to a new variable even would technically then be copying.

We're getting the best of both worlds; and simply following the known CoW semantics that are frequent in Swift.

it used to be a "const reference" and so only the reference (pointer) was copied.

I dont know how expensive is copying of a small struct, and how much more expensive is allocating the space for it on heap rather than on stack; perhaps its not a real issue at all.

if the goal was to avoid copying of the lambda context, perhaps Context should have a reference to "static storage" and a reference to CoW "mutable storage" (the baggage). That way 2 references would always be copied but the content of the "static storage" would not need to be copied every time the baggage changes.

this does implement copy on write however changing of the storage is not thread safe which I think is not a nice API for user who would need to sync the changes himself

The operations in this piece of code are safe. If they weren't then Array and all of Swift's collections are screwed as well 😉 We are providing the same semantics as those types.

Thank you for rechecking it.
I do find it a bit confusing...

if I understand that it will be safe because set is mutating.

struct Context {

func notSafeUpdateBaggage(_ baggage: Baggage) {
 // update storage
}

// need to make a copy which will increase the storage reference counter
mutating func updateBaggage(_ baggage: Baggage) {
 // update storage
}

}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the goal was to avoid copying of the lambda context, perhaps Context should have a reference to "static storage" and a reference to CoW "mutable storage" (the baggage). That way 2 references would always be copied but the content of the "static storage" would not need to be copied every time the baggage changes.

Yeah if it becomes a problem I think we could do that 🤔 👍

I do find it a bit confusing...

if I understand that it will be safe because set is mutating.

Maybe this example can illustrate: Because this is a struct, in order to change it the function must be mutating (or set {} which implicitly is).

Then if anyone has:

let context = ...

they can't mutate it unless a second reference is created:

let context = ...
var changeMe = context

which ensures that we'll have the +1 count there and will copy on mutation.

It still is possible to make bugs like:

class Me { 
var onlyReference: Lambda.Context = ... 
for t in threads { 
  t.run { onlyReference.mutate() }
}

that remains illegal as usual, but not because of the CoW or Context type being wrong but how the user uses it being unsafe. The same example is true for e.g. var int: Int which you'd try to mutate +=1 from many threads. There's nothing wrong about the Int type, but about the unsafe modifications to where we store it.

self.storage._logger.updateMetadata(previous: self.storage.baggage, latest: newValue)
self.storage.baggage = newValue
} else {
var logger = self.storage._logger
logger.updateMetadata(previous: self.storage.baggage, latest: newValue)
self.storage = Storage(
baggage: newValue,
invokedFunctionARN: self.storage.invokedFunctionARN,
deadline: self.storage.deadline,
cognitoIdentity: self.storage.cognitoIdentity,
clientContext: self.storage.clientContext,
logger: self.storage._logger,
eventLoop: self.storage.eventLoop,
allocator: self.storage.allocator
)
}
}
}

/// The request ID, which identifies the request that triggered the function invocation.
public let requestID: String
public var requestID: String {
self.storage.baggage.lambdaRequestID
}

/// The AWS X-Ray tracing header.
public let traceID: String
public var traceID: String {
self.storage.baggage.lambdaTraceID
}

/// The ARN of the Lambda function, version, or alias that's specified in the invocation.
public let invokedFunctionARN: String
public var invokedFunctionARN: String {
self.storage.invokedFunctionARN
}

/// The timestamp that the function times out
public let deadline: DispatchWallTime
public var deadline: DispatchWallTime {
self.storage.deadline
}

/// For invocations from the AWS Mobile SDK, data about the Amazon Cognito identity provider.
public let cognitoIdentity: String?
public var cognitoIdentity: String? {
self.storage.cognitoIdentity
}

/// For invocations from the AWS Mobile SDK, data about the client application and device.
public let clientContext: String?
public var clientContext: String? {
self.storage.clientContext
}

/// `Logger` to log with
/// `Logger` to log with, it is automatically populated with `baggage` information (such as `traceID` and `requestID`).
///
/// - note: The `LogLevel` can be configured using the `LOG_LEVEL` environment variable.
public let logger: Logger
/// - note: The default `Logger.LogLevel` can be configured using the `LOG_LEVEL` environment variable.
public var logger: Logger {
get {
self.storage._logger
}
set {
if isKnownUniquelyReferenced(&self.storage) {
self.storage._logger = newValue
self.storage._logger.updateMetadata(previous: .topLevel, latest: self.storage.baggage)
}
}
}

/// The `EventLoop` the Lambda is executed on. Use this to schedule work with.
/// This is useful when implementing the `EventLoopLambdaHandler` protocol.
///
/// - note: The `EventLoop` is shared with the Lambda runtime engine and should be handled with extra care.
/// Most importantly the `EventLoop` must never be blocked.
public let eventLoop: EventLoop
public var eventLoop: EventLoop {
self.storage.eventLoop
}

/// `ByteBufferAllocator` to allocate `ByteBuffer`
/// This is useful when implementing `EventLoopLambdaHandler`
public let allocator: ByteBufferAllocator
public var allocator: ByteBufferAllocator {
self.storage.allocator
}

internal init(requestID: String,
traceID: String,
Expand All @@ -93,20 +186,20 @@ extension Lambda {
logger: Logger,
eventLoop: EventLoop,
allocator: ByteBufferAllocator) {
self.requestID = requestID
self.traceID = traceID
self.invokedFunctionARN = invokedFunctionARN
self.cognitoIdentity = cognitoIdentity
self.clientContext = clientContext
self.deadline = deadline
// utility
self.eventLoop = eventLoop
self.allocator = allocator
// mutate logger with context
var logger = logger
logger[metadataKey: "awsRequestID"] = .string(requestID)
logger[metadataKey: "awsTraceID"] = .string(traceID)
self.logger = logger
var baggage = Baggage.topLevel
baggage.lambdaRequestID = requestID
baggage.lambdaTraceID = traceID
self.storage = Storage(
baggage: baggage,
invokedFunctionARN: invokedFunctionARN,
deadline: deadline,
cognitoIdentity: cognitoIdentity,
clientContext: clientContext,
// utility
logger: logger,
eventLoop: eventLoop,
allocator: allocator
Copy link
Contributor

Choose a reason for hiding this comment

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

iirc evenloop and allocator are immutable and dont need to be at the storage level imo

Copy link
Contributor

Choose a reason for hiding this comment

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

or is this or allow them to be as part of the by-ref trick?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this is to keep the outer Context type small -- just a single pointer, otherwise we'd grow the struct and each time it gets let or passed somewhere those references would be copied -- this way, everything in storage, nothing gets copied other than the single pointer just when passing the context around.

)
}

public func getRemainingTime() -> TimeAmount {
Expand Down Expand Up @@ -146,3 +239,42 @@ extension Lambda {
}
}
}

// MARK: - Baggage Items

extension Baggage {

// MARK: - Baggage: RequestID

enum LambdaRequestIDKey: Key {
typealias Value = String
static var name: String? { AmazonHeaders.requestID }
}

/// The request ID, which identifies the request that triggered the function invocation.
public internal(set) var lambdaRequestID: String {
get {
self[LambdaRequestIDKey.self]! // !-safe, the runtime guarantees to always set an identifier, even in testing
}
set {
self[LambdaRequestIDKey.self] = newValue
}
}

// MARK: - Baggage: TraceID

enum LambdaTraceIDKey: Key {
typealias Value = String
static var name: String? { AmazonHeaders.traceID }
}

/// The AWS X-Ray tracing header.
public internal(set) var lambdaTraceID: String {
get {
self[LambdaTraceIDKey.self]! // !-safe, the runtime guarantees to always set an identifier, even in testing
}
set {
self[LambdaTraceIDKey.self] = newValue
}
}
}
2 changes: 1 addition & 1 deletion Sources/AWSLambdaRuntimeCore/LambdaRunner.swift
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ extension Lambda {
}

private extension Lambda.Context {
convenience init(logger: Logger, eventLoop: EventLoop, allocator: ByteBufferAllocator, invocation: Lambda.Invocation) {
init(logger: Logger, eventLoop: EventLoop, allocator: ByteBufferAllocator, invocation: Lambda.Invocation) {
self.init(requestID: invocation.requestID,
traceID: invocation.traceID,
invokedFunctionARN: invocation.invokedFunctionARN,
Expand Down
33 changes: 32 additions & 1 deletion Tests/AWSLambdaRuntimeTests/Lambda+CodeableTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import Logging
import NIO
import NIOFoundationCompat
import XCTest
import Baggage

class CodableLambdaTest: XCTestCase {
var eventLoopGroup: EventLoopGroup!
Expand Down Expand Up @@ -63,7 +64,22 @@ class CodableLambdaTest: XCTestCase {
XCTAssertEqual(response?.requestId, request.requestId)
}

// convencience method
func testBaggageContextInteractions() {
var context = newContext()
context.baggage.testValue = "hello"

context.logger.info("Test")
XCTAssertEqual(context.baggage.testValue, "hello")
XCTAssertEqual(context.baggage.lambdaTraceID, "abc123")
XCTAssertEqual(context.baggage.lambdaTraceID, context.traceID)
XCTAssertEqual(context.baggage.lambdaRequestID, context.requestID)

XCTAssertEqual("\(context.logger[metadataKey: "LambdaTraceIDKey"]!)", context.traceID)
XCTAssertEqual("\(context.logger[metadataKey: "LambdaRequestIDKey"]!)", context.requestID)
XCTAssertEqual("\(context.logger[metadataKey: "TestKey"]!)", context.baggage.testValue)
}

// convenience method
func newContext() -> Lambda.Context {
Lambda.Context(requestID: UUID().uuidString,
traceID: "abc123",
Expand All @@ -90,3 +106,18 @@ private struct Response: Codable, Equatable {
self.requestId = requestId
}
}

extension Baggage {
enum TestKey: Baggage.Key {
typealias Value = String

}
var testValue: String? {
get {
return self[TestKey.self]
}
set {
self[TestKey.self] = newValue
}
}
}