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

fix(metadata): mutex for concurrency #176

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

just1not2
Copy link

@just1not2 just1not2 commented Dec 11, 2024

Description

This PR brings a fix for containerd/containerd#11138

It consists in a tiny change of the ttrpc.MD object to make it safer by preventing the access of its content from outside this project and adding a RW mutex.

Release plan

Here are the projects that are consuming ttrpc and need to be updated:

@just1not2 just1not2 force-pushed the fix-metatada-concurrency branch from fa7f73d to 773b104 Compare December 11, 2024 14:51
@just1not2 just1not2 marked this pull request as ready for review December 11, 2024 15:07
Comment on lines +26 to +29
type MD struct {
data map[string][]string
mu sync.RWMutex
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this suppose to be used like a http.Header where you need to Clone when sharing with other "requests", or suppose to be shared and need a mutex?

Copy link
Author

Choose a reason for hiding this comment

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

I think both ways are fine and we just need to stick with one, I just think the change will be easier to operate with the mutex solution given that there are MD objects that are mutated with the Set and Append (and setRequest) functions (like here)

@cpuguy83
Copy link
Member

This looks more like a mishandling of metadata and not something we should need locks for.

@cpuguy83
Copy link
Member

I think if GetMetadata returns a copy instead of the actual map that would resolve this?

@just1not2
Copy link
Author

This looks more like a mishandling of metadata and not something we should need locks for.
I think if GetMetadata returns a copy instead of the actual map that would resolve this?

As @djdongjin suggested, I think there are 2 ways of dealing with this issue: we can either make the MD object non-mutable or use mutexes to make sure the projects that are consuming containerd/ttrpc cannot reach concurrent operation issues.

Given that the current implementation already relies on updating the MD objects in place (example here and here), I think it would take less effort to implement the second solution (hence my proposal) but I'm not strongly opinionated on the matter.

I would however not recommend only changing the GetMetadata function as it would not prevent concurrent operation issues from the Set and Append functions and map iterations...

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.

3 participants