Skip to content

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

Open
@alshopov

Description

@alshopov

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions