Embed mutex in the server's websocket client #200
Merged
Conversation
This file contains 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 will prevent concurrent writes both in server implementations that store a reference to the client from the OnConnectedFunc callback. Concurrent writes can either occur when the server library responds to agent messages or when the implementation tries to send messages to the server on different threads.
Codecov ReportPatch coverage is
📢 Thoughts on this report? Let us know!. |
evan-bradley
commented
Sep 15, 2023
| case <-timeout.Done(): | ||
| t.Error("Client failed to connect before timeout") | ||
| default: | ||
| if _, ok := srvConnVal.Load().(types.Connection); ok == true { |
There was a problem hiding this comment.
I get a linter warning that ok is unused if I just have ok as the if-statement condition. ok == true fixes the warning, not sure how else to address this.
tigrannajaryan
approved these changes
Sep 19, 2023
|
Hint: https://pkg.go.dev/golang.org/x/perf/cmd/benchstat is very useful for showing before/after benchmark comparison. Here is what it produces for your benchmark results: Looks like it is close, but could use a more stable execution environment for benchmarking. :-) |
andykellr
approved these changes
Sep 20, 2023
|
@evan-bradley opamp-go v0.9.0 released: https://github.com/open-telemetry/opamp-go/releases/tag/v0.9.0 |
evan-bradley
added a commit
to open-telemetry/opentelemetry-collector-contrib
that referenced
this pull request
Dec 12, 2023
**Description:** Add tests that verify the Supervisor's behavior using a real Collector binary against an OpAMP server written with the opamp-go library. This tests a basic set of functionality mostly based on what is currently listed as implemented in the Supervisor's feature table. Right now these tests start the Supervisor using the Go API to avoid process handling, but could be changed to start it through a binary in the future. I've placed these tests in the E2E testing workflow even though they run fairly quickly since they depend on a built Collector binary, may become more expansive in the future, and don't fit in any of the existing jobs in the `build-and-test` workflow. I'm open to placing them in another location if we'd prefer them elsewhere. This also updates opamp-go to v0.9.0 to take advantage of open-telemetry/opamp-go#200. **Link to tracking Issue:** Resolves #24292
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The Gorilla websocket package states that only one concurrent write can be made to a connection at a time. The library will try to detect concurrent writes and will panic if it detects that one is happening.
We might see concurrent writes in two places:
OnConnectedFunc.In my opinion it would make sense to prevent concurrent writes within the library to relieve the application from having to worry about this detail. This does necessitate a mutex per connection, but I don't see any significant performance impact from the mutexes.
Without the mutex in
wsConnection:With the mutex:
This is a fairly rudimentary solution, but it does solve the issue I was seeing and is straightforward. I'm open to more sophisticated ways of managing the write concurrency if there's a deficiency with this approach.