disable protocol validation check before sending a Subscribe() #10
+19
−14
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
LookupMetadata both reads from and writes to a raw go map. Concurrent calls to LookupMetadata are therefore unsafe, as concurrent reads from and writes to the same map are forbidden. This results in runtime panics in practice. sync.Map is optimized for maps that only ever grow, and is safe to access concurrently. Switching the measurementRegistry to a sync.Map provides for atomic accesses.
allow multiple simultaneous calls to LookupMetadata
|
Do you know what version of STTP your publisher is using, note the following: https://github.com/sttp/goapi/blob/main/sttp/transport/DataSubscriber.go#L812 |
Taking the pointer to an item created in scope in a way that it can
escape the function in which it is contained requires putting it on the
heap.
As such, LoadOrStore(someMap, &Foo{}) constructs an unneeded item on
every load. A single memory allocation isn't hugely expensive, but
LookupMetadata is a _very_ hot function, and ends up both allocating
enough to be using nontrivial amounts of CPU, as well as applying GC
pressure.
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.
Creating a datasubscribe with AutoRequestMetadata disabled has been broken since 506f15d, which added the reverse connection support. The connection is not validated when we go to Subscribe(), and so the subscribe does not get sent, and the response does not get received, so the connection is not marked as valid.
We've been hacking around this by using AutoRequestMetadata even where it's unwanted, but we have reason to believe that having that enabled on the data worker may be inflating RAM usage.
I don't think this is the correct solution; it's probably better to have the no-op Subscribe response result in the connection being considered valid? Posting this only to begin that discussion :)