-
Notifications
You must be signed in to change notification settings - Fork 3
fix(go): implement StateHandler for proper shutdown #109
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
Merged
Merged
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
9cd6dc5
fix(go): implement StateHandler for proper shutdown
nicklasl 6a1d766
fixup! fix(go): implement StateHandler for proper shutdown
nicklasl 1c8da12
fixup! fixup! fix(go): implement StateHandler for proper shutdown
nicklasl 12302c7
test: add integration test for shutdown
nicklasl c277c3f
fixup! test: add integration test for shutdown
nicklasl 83a7386
fix(go): fixes from PR review
nicklasl 785a636
fix(go): add lock to prevent concurrent shutdowns
nicklasl 1008c8d
refactor(go): clean up cancelfunc stuff
nicklasl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
171 changes: 171 additions & 0 deletions
171
openfeature-provider/go/confidence/local_resolver_factory_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,171 @@ | ||
| package confidence | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| resolverv1 "github.com/spotify/confidence-resolver/openfeature-provider/go/confidence/proto/confidence/flags/resolverinternal" | ||
| ) | ||
|
|
||
| // mockWasmFlagLoggerForFactory is a mock implementation for testing factory shutdown | ||
| type mockWasmFlagLoggerForFactory struct { | ||
| shutdownCalled bool | ||
| writeCalled bool | ||
| onShutdown func() | ||
| onWrite func(ctx context.Context, request *resolverv1.WriteFlagLogsRequest) error | ||
| } | ||
|
|
||
| func (m *mockWasmFlagLoggerForFactory) Write(ctx context.Context, request *resolverv1.WriteFlagLogsRequest) error { | ||
| m.writeCalled = true | ||
| if m.onWrite != nil { | ||
| return m.onWrite(ctx, request) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (m *mockWasmFlagLoggerForFactory) Shutdown() { | ||
| m.shutdownCalled = true | ||
| if m.onShutdown != nil { | ||
| m.onShutdown() | ||
| } | ||
| } | ||
|
|
||
| func TestLocalResolverFactory_ShutdownOrder(t *testing.T) { | ||
| // Track the order in which shutdown methods are called | ||
| var callOrder []string | ||
|
|
||
| mockLogger := &mockWasmFlagLoggerForFactory{ | ||
| onShutdown: func() { | ||
| callOrder = append(callOrder, "logger") | ||
| }, | ||
| } | ||
|
|
||
| factory := &LocalResolverFactory{ | ||
| cancelFunc: func() { | ||
| callOrder = append(callOrder, "cancel") | ||
| }, | ||
| flagLogger: mockLogger, | ||
| resolverAPI: nil, | ||
| } | ||
|
|
||
| ctx := context.Background() | ||
| factory.Shutdown(ctx) | ||
|
|
||
| // Verify shutdown was called | ||
| if !mockLogger.shutdownCalled { | ||
| t.Error("Expected flag logger Shutdown to be called") | ||
| } | ||
|
|
||
| // Verify order: cancel should be called before logger shutdown | ||
| if len(callOrder) != 2 { | ||
| t.Errorf("Expected 2 shutdown calls, got %d", len(callOrder)) | ||
| } | ||
| if len(callOrder) >= 2 { | ||
| if callOrder[0] != "cancel" { | ||
| t.Errorf("Expected cancel to be called first, but got %s", callOrder[0]) | ||
| } | ||
| if callOrder[1] != "logger" { | ||
| t.Errorf("Expected logger to be called second, but got %s", callOrder[1]) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // mockResolverAPI is a mock implementation for testing shutdown order | ||
| type mockResolverAPI struct { | ||
| closeCalled bool | ||
| onClose func() | ||
| } | ||
|
|
||
| func (m *mockResolverAPI) Close(ctx context.Context) { | ||
| m.closeCalled = true | ||
| if m.onClose != nil { | ||
| m.onClose() | ||
| } | ||
| } | ||
|
|
||
| func TestLocalResolverFactory_ShutdownOrderWithResolver(t *testing.T) { | ||
| // This test verifies the critical shutdown order: | ||
| // 1. Cancel context | ||
| // 2. Wait for background tasks | ||
| // 3. Close resolver API (which flushes final logs) | ||
| // 4. Shutdown logger (which waits for log sends to complete) | ||
| // | ||
| // This order ensures logs generated during resolver Close are actually sent. | ||
|
|
||
| var callOrder []string | ||
| var logsSent bool | ||
|
|
||
| mockLogger := &mockWasmFlagLoggerForFactory{ | ||
| onWrite: func(ctx context.Context, request *resolverv1.WriteFlagLogsRequest) error { | ||
| callOrder = append(callOrder, "logger-write") | ||
| logsSent = true | ||
| return nil | ||
| }, | ||
| onShutdown: func() { | ||
| callOrder = append(callOrder, "logger-shutdown") | ||
| // At this point, logs should already be sent | ||
| if !logsSent { | ||
| t.Error("Logger shutdown called before logs were sent!") | ||
| } | ||
| }, | ||
| } | ||
|
|
||
| mockResolver := &mockResolverAPI{ | ||
| onClose: func() { | ||
| callOrder = append(callOrder, "resolver-close") | ||
| // Simulate resolver flushing logs on close | ||
| mockLogger.Write(context.Background(), &resolverv1.WriteFlagLogsRequest{}) | ||
| }, | ||
| } | ||
|
|
||
| factory := &LocalResolverFactory{ | ||
| cancelFunc: func() { | ||
| callOrder = append(callOrder, "cancel") | ||
| }, | ||
| flagLogger: mockLogger, | ||
| resolverAPI: (*SwapWasmResolverApi)(nil), // Can't easily mock this, test order instead | ||
| } | ||
|
|
||
| // Manually test the shutdown sequence - simulating the CORRECT order | ||
| // This test verifies our fix works correctly | ||
|
|
||
| if factory.cancelFunc != nil { | ||
| factory.cancelFunc() | ||
| } | ||
|
|
||
| // Wait for background tasks (part of our fix) | ||
| factory.wg.Wait() | ||
|
|
||
| // Close resolver FIRST (which generates logs) | ||
| mockResolver.Close(context.Background()) | ||
|
|
||
| // Then shutdown logger (which waits for logs to be sent) | ||
| if factory.flagLogger != nil { | ||
| factory.flagLogger.Shutdown() | ||
| } | ||
|
|
||
| // Verify the CORRECT order: cancel → resolver-close → logger-write → logger-shutdown | ||
| expectedOrder := []string{"cancel", "resolver-close", "logger-write", "logger-shutdown"} | ||
| if len(callOrder) != len(expectedOrder) { | ||
| t.Errorf("Expected %d calls, got %d: %v", len(expectedOrder), len(callOrder), callOrder) | ||
| } | ||
|
|
||
| for i, expected := range expectedOrder { | ||
| if i < len(callOrder) && callOrder[i] != expected { | ||
| t.Errorf("Expected call %d to be '%s', got '%s'", i, expected, callOrder[i]) | ||
| } | ||
| } | ||
|
|
||
| // Verify logs were sent before logger shutdown | ||
| if !logsSent { | ||
| t.Error("Expected logs to be sent during shutdown") | ||
| } | ||
|
|
||
| // Verify all components were called | ||
| if !mockResolver.closeCalled { | ||
| t.Error("Expected resolver Close to be called") | ||
| } | ||
| if !mockLogger.shutdownCalled { | ||
| t.Error("Expected logger Shutdown to be called") | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -299,7 +299,14 @@ func (p *LocalResolverProvider) Hooks() []openfeature.Hook { | |
| return []openfeature.Hook{} | ||
| } | ||
|
|
||
| // Shutdown closes the provider and cleans up resources | ||
| // Init initializes the provider (part of StateHandler interface) | ||
| func (p *LocalResolverProvider) Init(evaluationContext openfeature.EvaluationContext) error { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The absense of this function was the reason that |
||
| // Provider is already initialized in NewProvider, nothing to do here | ||
| // TODO move the bulk of the initialization to this place. | ||
| return nil | ||
| } | ||
|
|
||
| // Shutdown closes the provider and cleans up resources (part of StateHandler interface) | ||
| func (p *LocalResolverProvider) Shutdown() { | ||
| if p.factory != nil { | ||
| p.factory.Shutdown(context.Background()) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.