Reading from map with locks doesn't return value via channel

Question

I tried to implement a locking version of reading/writing from a map in golang, but it doesn't return the desired result.

package main

import (
    "sync"
    "fmt"
)

var m = map[int]string{}
var lock = sync.RWMutex{}

func StoreUrl(id int, url string) {
        for {
                lock.Lock()
                defer lock.Unlock()

                m[id] = url
        }
}

func LoadUrl(id int, ch chan string) {
    for {
        lock.RLock()
        defer lock.RUnlock()

        r := m[id]
        ch <- r
    }
}

func main() {
    go StoreUrl(125, "www.google.com")

    chb := make(chan string)
    go LoadUrl(125, chb);

    C := <-chb
    fmt.Println("Result:", C)                           
}

The output is:

Result:

Meaning the value is not returned via the channel, which I don't get. Without the locking/goroutines it seems to work fine. What did I do wrong?

The code can also be found here:

https://play.golang.org/p/-WmRcMty5B

why do you need an infinite loop to store one url ? and the same for LoadUrl? have you tried this code in your local machine ? — Yandry Pozo, Jan 11 '17 at 03:44
Yes I tried this code in my local machine, I took it largely froom a SO question, http://stackoverflow.com/a/38140573/132728 — Max, Jan 11 '17 at 03:45
@Max Both the infinite loops are unnecessary. They will cause [goroutine leakage](https://dave.cheney.net/2016/12/22/never-start-a-goroutine-without-knowing-how-it-will-stop). — John S Perayil, Jan 11 '17 at 05:49

Ankur · Accepted Answer · 2017-01-11T04:42:53.183

Infinite loops without sleep or some kind of IO are always bad idea.

In your code if you put a print statement at the start of StoreUrl, you will find that it never gets printed i.e the go routine was never started, the go call is setting putting the info about this new go routine in some run queue of the go scheduler but the scheduler hasn't ran yet to schedule that task. How do you run the scheduler? Do sleep/IO/channel reading/writing.

Another problem is that your infinite loop is taking lock and trying to take the lock again, which will cause it to deadlock. Defer only run after function exit and that function will never exit because of infinite loop.

Below is modified code that uses sleep to make sure every execution thread gets time to do its job.

package main

import (
    "sync"
    "fmt"
    "time"
)

var m = map[int]string{}
var lock = sync.RWMutex{}

func StoreUrl(id int, url string) {
        for {
                lock.Lock()
                m[id] = url
                lock.Unlock()
                time.Sleep(1)
        }
}

func LoadUrl(id int, ch chan string) {
    for {
            lock.RLock()
            r := m[id]
            lock.RUnlock()
            ch <- r

    }
}

func main() {
    go StoreUrl(125, "www.google.com")
    time.Sleep(1)
    chb := make(chan string)
    go LoadUrl(125, chb);

    C := <-chb
    fmt.Println("Result:", C)
}

Edit: As @Jaun mentioned in the comment, you can also use runtime.Gosched() instead of sleep.

What about replacing the sleep instruction for runtime.Gosched()? it would yield the processor and would be more meaningful than just sleeping https://godoc.org/runtime#Gosched — Juan Carlos Garcia, Jan 11 '17 at 04:37
What's the difference between this code and my code w/r to the infinite loops? Dont they also cause "goroutine leakage", as some dude commented? — Max, Jan 23 '17 at 08:01

score 1 · Answer 2 · answered Jan 11 '17 at 04:33

1

Usage of defer incorrect, defer execute at end of function, not for statement.

func StoreUrl(id int, url string) {
    for {
        func() {
            lock.Lock()
            defer lock.Unlock()
            m[id] = url
        }()
    }
}

or

func StoreUrl(id int, url string) {
    for {
        lock.Lock()
        m[id] = url
        lock.Unlock()
    }
}

We can't control the order of go routine, so add time.Sleep() to control the order.

code here:

https://play.golang.org/p/Bu8Lo46SA2

answered Jan 11 '17 at 04:33

blue-white

11
1

I don't know why but your solution caused hanging of the applicatin when used in a server application. – Max Jan 11 '17 at 06:37
add time.Sleep() in every for statement(https://play.golang.org/p/-zzDfFIq-S) or run in your own machine – blue-white Jan 11 '17 at 06:59

Reading from map with locks doesn't return value via channel

2 Answers2