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

Streaming connections are now closed when you call Close on a client #79

Merged
merged 1 commit into from
Apr 7, 2022

Conversation

jcox250
Copy link
Contributor

@jcox250 jcox250 commented Apr 6, 2022

What

Close wasn't closing streaming connections which meant they would stay open forever.

Testing

Started up the sample sdk locally with it pointing at this version of the SDK and had it stream for a period of time before calling Close. Before Close was called I could see streamed events in the logs when I toggled a flag on/off and after Close was called I was not seeing streamed events in the logs whenever I toggled a flag on/off

The unit test for this is a bit light but I can't see a way to do anything more thorough without extensively refactoring the code in the client. Ideally if we could break the logic in the client out into smaller components we would be able to define some interfaces for the separate bits of logic that the client is doing e.g.

type streamer interface {
   // Handle streaming connections & business logic
   Stream()
}

type authenticator interface {
   // Perform inital auth and retries
   Authenticate()
}

// Some wrapper around the analyticsService methods we use
type analyticsService interface {
}

type CfClient struct {
   stream streamer
   auth authenticator
   analytics analyticsService
}

Then we could have some mocks for these which would let us verify that the Close functionality is actually sending a close signal to each of these components.

client/client.go Outdated Show resolved Hide resolved
Copy link
Contributor

@enver-bisevac enver-bisevac left a comment

Choose a reason for hiding this comment

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

LGTM, minor comments on custom safe bool implementation

@jcox250 jcox250 merged commit 52b0e78 into main Apr 7, 2022
@jcox250 jcox250 deleted the FFM-1943 branch April 7, 2022 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants