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

Both production and mock implementation of inbound calls are thred unsafe #2357

Open
alshopov opened this issue Feb 24, 2025 · 1 comment · May be fixed by #2358
Open

Both production and mock implementation of inbound calls are thred unsafe #2357

alshopov opened this issue Feb 24, 2025 · 1 comment · May be fixed by #2358

Comments

@alshopov
Copy link

alshopov commented Feb 24, 2025

For short demo see this pull request: #2356

The underlying problem in both cases are mutable fields that can be changed in separate go routines.

A minimal fix for the mock implementation is a sentence in the docs. Note that the existing API hints on the defenestrations - basically yarpctest.ContextWithCall takes a map of string to string for the headers. By default maps should not be mutated and once can reasonably say that this is expected.

For the production one this is suboptimal. We can start with warning about the behavior. The api seems to hint that the call is thread safe - for example Call.OriginalHeaders says it is returning a copy of headers. The only other API that returns a mutrable object can similarly be supposed it is returning a copy: Call.HeaderNames

Fixing this in a place outside the Call implementation will be impossible. Therefore this needs to be pointed out in the docs.

It is possible to fix this by guarding the access to the ic.resHeaders with a lock. The cost is more memory used plus some contention.

I will create another pull request with a fix using a single lock. I do not think going the full way of using a read-write lock is worth it - headers are not massively more read than mutated and it makes no sense to do that.


As promised - here is the propose fix: #2358


Another thing that may be pointed out in the documentation - Call serves as global object for the whole context chain within a service. Changes made to the object will be visible to separate places in the call chain.

@biosvs
Copy link
Collaborator

biosvs commented Feb 24, 2025

It's true, yarpc is not safe thread in many ways. We also have body field that we should be careful with.

I agree though that documentation should state it explicitly.

We probably also want to implement a "Clone" helper function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants