Skip to content

Commit

Permalink
add smithy-go container LRU impl and debounce returns in v2 endpoint …
Browse files Browse the repository at this point in the history
…resolution (#443)
  • Loading branch information
lucix-aws authored Aug 7, 2023
1 parent 3714cee commit bf1287c
Show file tree
Hide file tree
Showing 7 changed files with 174 additions and 15 deletions.
8 changes: 8 additions & 0 deletions .changelog/d2db122d1fda4d9e85a8f482949f7e3e.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"id": "d2db122d-1fda-4d9e-85a8-f482949f7e3e",
"type": "bugfix",
"description": "Prevent duplicated error returns in EndpointResolverV2 default implementation.",
"modules": [
"."
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,11 @@ private void writeConfigFieldResolvers(
writer.writeInline(", opID");
}
if (resolver.isWithClientInput()) {
writer.writeInline(", *c");
if (resolver.getLocation() == ConfigFieldResolver.Location.CLIENT) {
writer.writeInline(", client");
} else {
writer.writeInline(", *c");
}
}
writer.write(")");
writer.write("");
Expand Down Expand Up @@ -153,17 +157,17 @@ private void generateConstructor(Symbol serviceSymbol) {
writer.openBlock("for _, fn := range optFns {", "}", () -> writer.write("fn(&options)"));
writer.write("");

writer.openBlock("client := &$T{", "}", serviceSymbol, () -> {
writer.write("options: options,");
}).write("");

// Run any config finalization functions registered by runtime plugins.
for (RuntimeClientPlugin plugin : plugins) {
writeConfigFieldResolvers(writer, plugin, resolver ->
resolver.getLocation() == ConfigFieldResolver.Location.CLIENT
&& resolver.getTarget() == ConfigFieldResolver.Target.FINALIZATION);
}

writer.openBlock("client := &$T{", "}", serviceSymbol, () -> {
writer.write("options: options,");
}).write("");

// Run any client member resolver functions registered by runtime plugins.
for (RuntimeClientPlugin plugin : plugins) {
writeClientMemberResolvers(writer, plugin, resolver -> true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,21 +225,17 @@ private GoWriter.Writable generateRulesList(List<Rule> rules, Scope scope) {
});

if (!rules.isEmpty() && !(rules.get(rules.size() - 1).getConditions().isEmpty())) {
// the rules tree can be constructed in a manner that tricks us
// into writing this twice consecutively (we write it, return
// to the outer generateRulesList call, and write it again
// without having closed the outer parentheses
//
// ensure we write only once to avoid unreachable code, but
// this should most likely be handled/fixed elsewhere
// FIXME: there's currently a bug in traversal that sometimes causes subsequent returns to be generated
// which fails vet. Patch over it via debounce for now
ensureFinalTreeRuleError(w);
}
};
}

private void ensureFinalTreeRuleError(GoWriter w) {
final String expected = "return endpoint, fmt.Errorf(\"" + ERROR_MESSAGE_ENDOFTREE + "\")";
if (!w.toString().trim().endsWith(expected)) {
final String[] lines = w.toString().split("\n");
final String lastLine = lines[lines.length - 1];
if (!lastLine.trim().startsWith("return")) {
w.writeGoTemplate(
"return endpoint, $fmtErrorf:T(\"" + ERROR_MESSAGE_ENDOFTREE + "\")",
commonCodegenArgs
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public boolean isWithOperationName() {
}

public boolean isWithClientInput() {
return withClientInput && location == Location.OPERATION;
return withClientInput;
}

public static Builder builder() {
Expand Down
19 changes: 19 additions & 0 deletions container/private/cache/cache.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Package cache defines the interface for a key-based data store.
//
// This package is designated as private and is intended for use only by the
// smithy client runtime. The exported API therein is not considered stable and
// is subject to breaking changes without notice.
package cache

// Cache defines the interface for an opaquely-typed, key-based data store.
//
// The thread-safety of this interface is undefined and is dictated by
// implementations.
type Cache interface {
// Retrieve the value associated with the given key. The returned boolean
// indicates whether the cache held a value for the given key.
Get(k interface{}) (interface{}, bool)

// Store a value under the given key.
Put(k interface{}, v interface{})
}
63 changes: 63 additions & 0 deletions container/private/cache/lru/lru.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
// Package lru implements [cache.Cache] with an LRU eviction policy.
//
// This implementation is NOT thread-safe.
//
// This package is designated as private and is intended for use only by the
// smithy client runtime. The exported API therein is not considered stable and
// is subject to breaking changes without notice.
package lru

import (
"container/list"

"github.com/aws/smithy-go/container/private/cache"
)

// New creates a new LRU cache with the given capacity.
func New(cap int) cache.Cache {
return &lru{
entries: make(map[interface{}]*list.Element, cap),
cap: cap,
mru: list.New(),
}
}

type lru struct {
entries map[interface{}]*list.Element
cap int

mru *list.List // least-recently used is at the back
}

type element struct {
key interface{}
value interface{}
}

func (l *lru) Get(k interface{}) (interface{}, bool) {
e, ok := l.entries[k]
if !ok {
return nil, false
}

l.mru.MoveToFront(e)
return e.Value.(*element).value, true
}

func (l *lru) Put(k interface{}, v interface{}) {
if len(l.entries) == l.cap {
l.evict()
}

ev := &element{
key: k,
value: v,
}
e := l.mru.PushFront(ev)
l.entries[k] = e
}

func (l *lru) evict() {
e := l.mru.Remove(l.mru.Back())
delete(l.entries, e.(*element).key)
}
69 changes: 69 additions & 0 deletions container/private/cache/lru/lru_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
package lru

import "testing"

func TestCache(t *testing.T) {
cache := New(4).(*lru)

// fill cache
cache.Put(1, 2)
cache.Put(2, 3)
cache.Put(3, 4)
cache.Put(4, 5)
assertEntry(t, cache, 1, 2)
assertEntry(t, cache, 2, 3)
assertEntry(t, cache, 3, 4)
assertEntry(t, cache, 4, 5)

// touch the last entry
cache.Get(1)
cache.Put(5, 6)
assertNoEntry(t, cache, 2)
assertEntry(t, cache, 3, 4)
assertEntry(t, cache, 4, 5)
assertEntry(t, cache, 1, 2)
assertEntry(t, cache, 5, 6)

// put something new, 3 should now be the oldest
cache.Put(6, 7)
assertNoEntry(t, cache, 3)
assertEntry(t, cache, 4, 5)
assertEntry(t, cache, 1, 2)
assertEntry(t, cache, 5, 6)
assertEntry(t, cache, 6, 7)

// touch something in the middle
cache.Get(5)
assertEntry(t, cache, 4, 5)
assertEntry(t, cache, 1, 2)
assertEntry(t, cache, 5, 6)
assertEntry(t, cache, 6, 7)

// put 3 new things, 5 should remain after the touch
cache.Put(7, 8)
cache.Put(8, 9)
cache.Put(9, 0)
assertNoEntry(t, cache, 4)
assertNoEntry(t, cache, 1)
assertNoEntry(t, cache, 6)
assertEntry(t, cache, 5, 6)
assertEntry(t, cache, 7, 8)
assertEntry(t, cache, 8, 9)
assertEntry(t, cache, 9, 0)
}

func assertEntry(t *testing.T, c *lru, k interface{}, v interface{}) {
e, ok := c.entries[k]
if !ok {
t.Errorf("expected entry %v=%v, but no entry found", k, v)
}
if actual := e.Value.(*element).value; actual != v {
t.Errorf("expected entry %v=%v, but got entry value %v", k, v, actual)
}
}

func assertNoEntry(t *testing.T, c *lru, k interface{}) {
if _, ok := c.Get(k); ok {
t.Errorf("expected no entry for %v, but one was found", k)
}
}

0 comments on commit bf1287c

Please sign in to comment.