CODEWITHSUNDEEP
goconcurrencymutex
Leylandski
Leylandski

Reputation: 441

Do I need a mutex if I am returning a copy of the variable rather than a pointer?

I'm a little confused about the use of sync.Mutex in Go. I understand the basic concept (calling Lock() will prevent other goroutines from executing the code between it and Unlock()), but I'm not sure if I need it here. I've seen a fair few C++ answers for this but in each example they all seem to be modifying and accessing a variable directly.

This is my code from a package called configuration, which I will use throughout the application to get (surprisingly) configuration and settings information.

package config

import (
    "encoding/json"
    "fmt"
    "os"
    "sync"

    log "github.com/sirupsen/logrus"
)

/*
ConfigurationError is an implementation of the error interface describing errors that occurred while dealing with this
package.
*/
type ConfigurationError string

/*
Error prints the error message for this ConfigurationError. It also implements the error interface.
*/
func (ce ConfigurationError) Error() string {
    return fmt.Sprintf("Configuration error: %s", string(ce))
}

/*
configuration is a data struct that holds all of the required information for setting up the program. It is unexported
to prevent other packages creating more instances. Other packages that need settings information should call Current()
to access a copy of the unexported programConfig package variable.
*/
type configuration struct {
    sync.RWMutex
    LogLevel               log.Level `json:"logLevel,omitempty"`     //Logging
    LogLocation            string    `json:"logLocation,omitempty"`  //-
    HttpPort               int       `json:"port,omitempty"`         //Web
    LoginUri               string    `json:"loginUri"`               //-
    WebBaseUri             string    `json:"webBaseUri"`             //-
    StaticBaseUri          string    `json:"staticBaseUri"`          //-
    ApiBaseUri             string    `json:"apiBaseUri"`             //-
    StaticContentLocalPath string    `json:"staticContentLocalPath"` //-
    MaxSimultaneousReports int       `json:"maxSimultaneousReports"` //Reporting
}

var programConfig configuration

/*
Current returns a copy of the currently loaded program configuration.
*/
func Current() configuration {
    programConfig.RLock()
    defer programConfig.RUnlock()
    return programConfig
}

/*
Load attempts to load a JSON settings file containing a representation of the Configuration struct. It will then set
the value of the package-level config struct to the loaded values. Some settings changes will require a restart of the
server application.

filepath - the full path of the settings file including a leading slash or drive name (depending on the OS).
*/
func Load(filepath string) error {

    //Open the file for reading.
    settingsFile, err := os.Open(filepath)
    if err != nil {
        return ConfigurationError(err.Error())
    }
    defer settingsFile.Close()

    //Decode JSON into package level var.
    decoder := json.NewDecoder(settingsFile)
    newSettings := configuration{}
    err = decoder.Decode(&newSettings)
    if err != nil {
        return ConfigurationError(err.Error())
    }

    programConfig.Lock() //I'm not 100% sure this is the correct use of a mutex for this situation, so check up on that.
    programConfig = newSettings
    programConfig.Unlock()
    return nil

}

As you can see, I've used mutex in two places.

Given that, am I using them correctly and why do I need one when reading a copy of the data (if so)?

Upvotes: 1

Views: 1484

Answers (1)

Christian
Christian

Reputation: 3721

When you read data which can be written at the same time you need a mutex. Otherwise it might happen that you read while it's written and get half of the old data and half of the new data.

So your example seems to be just fine. Because you are probably reading the config very often but writing it rarely your usage of a RWLock makes sense. This means that multiple threads can read at the same time as long as it's not written.

What in your code looks dangerous is:

programConfig.Lock()
programConfig = newSettings
programConfig.Unlock()

Because programConfig contains the Mutex you are doing the Lock and Unlock on different instances which will lead to deadlocks. You should add the Mutex to the "parent" of the instance. In this case probably the package.

Upvotes: 8

Related Questions