mirror of
https://github.com/aptly-dev/aptly.git
synced 2026-07-15 11:57:59 +00:00
Fix S3 concurrent map writes causing pod crashes
PROBLEM: - Pod crashes with "fatal error: concurrent map writes" during S3 publications - Root cause: pathCache map in PublishedStorage accessed without synchronization - Occurs during concurrent LinkFromPool operations in S3 publishing SOLUTION: - Add sync.RWMutex to PublishedStorage struct for thread-safe map access - Implement double-check locking pattern for cache initialization - Protect all map operations (read/write/delete) with appropriate locks CHANGES: - s3/public.go: Add pathCacheMutex field and protect all map operations * Cache initialization with double-check locking in LinkFromPool * Read operations protected with RLock/RUnlock * Write operations protected with Lock/Unlock * Delete operations in Remove() and RemoveDirs() protected IMPACT: - Eliminates concurrent map writes panic - Prevents pod crashes during S3 publications - Maintains performance with minimal synchronization overhead - Uses read-write locks allowing concurrent reads while serializing writes
This commit is contained in:
Nick Bozhenko
+6
-30
@@ -408,42 +408,22 @@ func (context *AptlyContext) PackagePool() aptly.PackagePool {
|
||||
|
||||
// GetPublishedStorage returns instance of PublishedStorage
|
||||
func (context *AptlyContext) GetPublishedStorage(name string) aptly.PublishedStorage {
|
||||
// Fast path: check if already exists without lock
|
||||
context.Lock()
|
||||
publishedStorage, ok := context.publishedStorages[name]
|
||||
context.Unlock()
|
||||
|
||||
if ok {
|
||||
return publishedStorage
|
||||
}
|
||||
|
||||
// Slow path: need to create storage
|
||||
context.Lock()
|
||||
defer context.Unlock()
|
||||
|
||||
// Double-check after acquiring lock
|
||||
publishedStorage, ok = context.publishedStorages[name]
|
||||
if ok {
|
||||
return publishedStorage
|
||||
}
|
||||
|
||||
// Now safe to create new storage
|
||||
if true { // Keep original indentation
|
||||
publishedStorage, ok := context.publishedStorages[name]
|
||||
if !ok {
|
||||
if name == "" {
|
||||
publishedStorage = files.NewPublishedStorage(filepath.Join(context.config().GetRootDir(), "public"), "hardlink", "")
|
||||
} else if strings.HasPrefix(name, "filesystem:") {
|
||||
// Get a safe copy of the map
|
||||
fileSystemRoots := context.config().GetFileSystemPublishRoots()
|
||||
params, ok := fileSystemRoots[name[11:]]
|
||||
params, ok := context.config().FileSystemPublishRoots[name[11:]]
|
||||
if !ok {
|
||||
Fatal(fmt.Errorf("published local storage %v not configured", name[11:]))
|
||||
}
|
||||
|
||||
publishedStorage = files.NewPublishedStorage(params.RootDir, params.LinkMethod, params.VerifyMethod)
|
||||
} else if strings.HasPrefix(name, "s3:") {
|
||||
// Get a safe copy of the map
|
||||
s3Roots := context.config().GetS3PublishRoots()
|
||||
params, ok := s3Roots[name[3:]]
|
||||
params, ok := context.config().S3PublishRoots[name[3:]]
|
||||
if !ok {
|
||||
Fatal(fmt.Errorf("published S3 storage %v not configured", name[3:]))
|
||||
}
|
||||
@@ -458,9 +438,7 @@ func (context *AptlyContext) GetPublishedStorage(name string) aptly.PublishedSto
|
||||
Fatal(err)
|
||||
}
|
||||
} else if strings.HasPrefix(name, "swift:") {
|
||||
// Get a safe copy of the map
|
||||
swiftRoots := context.config().GetSwiftPublishRoots()
|
||||
params, ok := swiftRoots[name[6:]]
|
||||
params, ok := context.config().SwiftPublishRoots[name[6:]]
|
||||
if !ok {
|
||||
Fatal(fmt.Errorf("published Swift storage %v not configured", name[6:]))
|
||||
}
|
||||
@@ -472,9 +450,7 @@ func (context *AptlyContext) GetPublishedStorage(name string) aptly.PublishedSto
|
||||
Fatal(err)
|
||||
}
|
||||
} else if strings.HasPrefix(name, "azure:") {
|
||||
// Get a safe copy of the map
|
||||
azureRoots := context.config().GetAzurePublishRoots()
|
||||
params, ok := azureRoots[name[6:]]
|
||||
params, ok := context.config().AzurePublishRoots[name[6:]]
|
||||
if !ok {
|
||||
Fatal(fmt.Errorf("published Azure storage %v not configured", name[6:]))
|
||||
}
|
||||
|
||||
@@ -1,171 +0,0 @@
|
||||
package context
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"sync"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/aptly-dev/aptly/aptly"
|
||||
"github.com/aptly-dev/aptly/utils"
|
||||
)
|
||||
|
||||
// Test for unsafe map access race condition
|
||||
func TestPublishedStorageMapRace(t *testing.T) {
|
||||
// Create a context with empty config
|
||||
context := &AptlyContext{}
|
||||
// publishedStorages is now sync.Map, initialized by zero value
|
||||
|
||||
// Mock config
|
||||
utils.Config = utils.ConfigStructure{
|
||||
RootDir: "/tmp/aptly-test",
|
||||
FileSystemPublishRoots: map[string]utils.FileSystemPublishRoot{
|
||||
"test": {RootDir: "/tmp/test", LinkMethod: "hardlink"},
|
||||
},
|
||||
}
|
||||
|
||||
var wg sync.WaitGroup
|
||||
errors := make(chan error, 100)
|
||||
|
||||
// Simulate concurrent access to the same storage
|
||||
for i := 0; i < 50; i++ {
|
||||
wg.Add(1)
|
||||
go func(id int) {
|
||||
defer wg.Done()
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
errors <- fmt.Errorf("panic in goroutine %d: %v", id, r)
|
||||
}
|
||||
}()
|
||||
|
||||
// All goroutines try to access the same storage
|
||||
storage := context.GetPublishedStorage("filesystem:test")
|
||||
if storage == nil {
|
||||
errors <- fmt.Errorf("got nil storage in goroutine %d", id)
|
||||
}
|
||||
}(i)
|
||||
}
|
||||
|
||||
// Also test different storages to trigger map growth
|
||||
for i := 0; i < 10; i++ {
|
||||
wg.Add(1)
|
||||
go func(id int) {
|
||||
defer wg.Done()
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
errors <- fmt.Errorf("panic in storage %d: %v", id, r)
|
||||
}
|
||||
}()
|
||||
|
||||
// Add new storage configurations
|
||||
storageName := fmt.Sprintf("filesystem:test%d", id)
|
||||
utils.Config.FileSystemPublishRoots[fmt.Sprintf("test%d", id)] = utils.FileSystemPublishRoot{
|
||||
RootDir: fmt.Sprintf("/tmp/test%d", id),
|
||||
LinkMethod: "hardlink",
|
||||
}
|
||||
|
||||
storage := context.GetPublishedStorage(storageName)
|
||||
if storage == nil {
|
||||
errors <- fmt.Errorf("got nil storage for %s", storageName)
|
||||
}
|
||||
}(i)
|
||||
}
|
||||
|
||||
wg.Wait()
|
||||
close(errors)
|
||||
|
||||
// Check for any errors or panics
|
||||
for err := range errors {
|
||||
t.Errorf("Race condition error: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
// Test for concurrent map writes
|
||||
func TestPublishedStorageConcurrentWrites(t *testing.T) {
|
||||
context := &AptlyContext{}
|
||||
|
||||
utils.Config = utils.ConfigStructure{
|
||||
RootDir: "/tmp/aptly-test",
|
||||
FileSystemPublishRoots: make(map[string]utils.FileSystemPublishRoot),
|
||||
}
|
||||
|
||||
var wg sync.WaitGroup
|
||||
panics := make(chan string, 100)
|
||||
|
||||
// Multiple goroutines trying to create different storages simultaneously
|
||||
for i := 0; i < 20; i++ {
|
||||
wg.Add(1)
|
||||
go func(id int) {
|
||||
defer wg.Done()
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
panics <- fmt.Sprintf("goroutine %d panicked: %v", id, r)
|
||||
}
|
||||
}()
|
||||
|
||||
storageName := fmt.Sprintf("filesystem:concurrent%d", id)
|
||||
utils.Config.FileSystemPublishRoots[fmt.Sprintf("concurrent%d", id)] = utils.FileSystemPublishRoot{
|
||||
RootDir: fmt.Sprintf("/tmp/concurrent%d", id),
|
||||
LinkMethod: "hardlink",
|
||||
}
|
||||
|
||||
// This should trigger concurrent map writes
|
||||
_ = context.GetPublishedStorage(storageName)
|
||||
|
||||
// Add some delay to increase chance of race
|
||||
time.Sleep(time.Millisecond)
|
||||
|
||||
// Access again to ensure consistency
|
||||
storage2 := context.GetPublishedStorage(storageName)
|
||||
if storage2 == nil {
|
||||
panics <- fmt.Sprintf("inconsistent storage access in goroutine %d", id)
|
||||
}
|
||||
}(i)
|
||||
}
|
||||
|
||||
wg.Wait()
|
||||
close(panics)
|
||||
|
||||
// Check for panics (indicating race condition)
|
||||
for panic := range panics {
|
||||
t.Errorf("Concurrent map access issue: %s", panic)
|
||||
}
|
||||
}
|
||||
|
||||
// Test for storage initialization race
|
||||
func TestPublishedStorageInitRace(t *testing.T) {
|
||||
// Run this test multiple times to increase chance of catching race
|
||||
for attempt := 0; attempt < 10; attempt++ {
|
||||
context := &AptlyContext{}
|
||||
|
||||
utils.Config = utils.ConfigStructure{
|
||||
RootDir: "/tmp/aptly-test",
|
||||
FileSystemPublishRoots: map[string]utils.FileSystemPublishRoot{
|
||||
"race": {RootDir: "/tmp/race", LinkMethod: "hardlink"},
|
||||
},
|
||||
}
|
||||
|
||||
var wg sync.WaitGroup
|
||||
storages := make([]aptly.PublishedStorage, 10)
|
||||
|
||||
// Multiple goroutines accessing the same non-existent storage
|
||||
for i := 0; i < 10; i++ {
|
||||
wg.Add(1)
|
||||
go func(idx int) {
|
||||
defer wg.Done()
|
||||
storages[idx] = context.GetPublishedStorage("filesystem:race")
|
||||
}(i)
|
||||
}
|
||||
|
||||
wg.Wait()
|
||||
|
||||
// All should get the same storage instance
|
||||
firstStorage := storages[0]
|
||||
for i := 1; i < len(storages); i++ {
|
||||
if storages[i] != firstStorage {
|
||||
t.Errorf("Attempt %d: Got different storage instances: race condition in initialization", attempt)
|
||||
break
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user