1
\$\begingroup\$

I'm writing a toy heartbeat server. Each client, upon loading, registers itself and thereafter, sends heartbeats every X seconds. The server checks every Y seconds if a heartbeat has been received.

(In the following code, X = 5sec and Y = 60sec.)

A brief description of the components.

Data

Users and their last heartbeat date-time are stored in a map.

Webserver

A web server goroutine serves the "web-app" for the client and handles AJAX requests for registration and heartbeats.

Registration AJAX: Each registration saves the user and current date-time to the map. It will start a goroutine that will check for a heartbeat every Y seconds.

Heartbeat AJAX: Each heartbeat saves the user and current date-time to the map.

Heartbeat check

A recursive function sleeps for Y seconds and check the user map if userID has had a heartbeat within the last Y seconds.

  • If there has been a heartbeat, the function will recurse for another check.
  • If there hasn't been a recent heartbeat, the user will be removed from the map and the checks will stop.
  • If the user does not exist in the map the checks will stop.

Client

The client will register upon loading. Thereafter, the client will send heartbeats over AJAX every X seconds.

The client has no knowledge of its registration state and will send heartbeats naively. This is done to simplify the toy code.

Questions I have

  • Is time.Sleep appropriate or should I user another construct such as time.Timer?
  • Is the handling of concurrency with map correct?
  • Relative* scalability given use of synchronization primitive for map?
  • Do goroutines, specifically the "child" ones, exit upon returning or does exit-ing need to be done explicitly?

* Reasonable scalability for one process. I understand if I want to go "primetime", the architecture should have a database backend to replace the map and have separate processes for web server and heartbeat checking.

Thank you for taking the time to review my code.

heartbeat.go

package main

import (
    "fmt"
    "log"
    "net/http"
    "sync"
    "time"
)

var users = make(map[string]time.Time)
var mutex = &sync.Mutex{}

func checkUser(userID string) {
    time.Sleep(60 * time.Second)

    mutex.Lock()
    registerTime, ok := users[userID]
    if ok && (time.Now().After(registerTime.Add(60 * time.Second))) {
        fmt.Println("Deleted", userID)
        delete(users, userID)
        mutex.Unlock()
    } else if ok {
        mutex.Unlock()
        fmt.Println("Heartbeat check passed for", userID)
        checkUser(userID)
    } else {
        mutex.Unlock()
        fmt.Println("End heartbeat check for", userID)
    }
}

func handleHeartbeat(w http.ResponseWriter, r *http.Request) {
    userIDArray, ok := r.Header["Userid"]
    fmt.Println("handleHeartbeat", time.Now())
    if ok {
        fmt.Println("Received heartbeat for", userIDArray[0], "at", time.Now())
        mutex.Lock()
        users[userIDArray[0]] = time.Now()
        mutex.Unlock()
    }
}

func handleRegister(w http.ResponseWriter, r *http.Request) {
    userIDArray, ok := r.Header["Userid"]
    fmt.Println("handleRegister", time.Now())
    if ok {
        fmt.Println("Received registration for", userIDArray[0], "at", time.Now())
        mutex.Lock()
        users[userIDArray[0]] = time.Now()
        mutex.Unlock()
        go checkUser(userIDArray[0])
    }
}

func main() {
    http.HandleFunc("/ajax/heartbeat", handleHeartbeat)
    http.HandleFunc("/ajax/register", handleRegister)
    http.Handle("/", http.FileServer(http.Dir("static")))
    err := http.ListenAndServe(":8080", nil)
    if err != nil {
        log.Fatal("ListenAndServe: ", err)
    }
}

static/index.html

<!DOCTYPE html>
<html>
  <head>
    <script src="https://ajax.googleapis.com/ajax/libs/jquery/3.2.1/jquery.min.js"></script>
  </head>
  <body>
  <script>
    $.ajax({
      url: '/ajax/register',
      type: 'POST',
      headers: {
        'userID': 'bobdole'
      },
      success: function() {
        window.setInterval(function() {
          $.ajax({
            url: '/ajax/heartbeat',
            type: 'POST',
            headers: {
              'userID': 'bobdole'
            }
          });
        }, 5000);
      }
    });
  </script>
  </body>
</html>
\$\endgroup\$
2
\$\begingroup\$

Quick design overview: I see no reason to spawn a new goroutine per user (edit: as hoffmale pointed out in the comments, it's at least one per user, but can be a lot more, as the concept of "user" is weakly-defined). This seems wasteful (even though goroutines are cheap). I'd go with having a single goroutine that periodically loops through the whole map and makes all the checks.

To answer your questions:

Is time.Sleep appropriate or should I user another construct such as time.Timer?

time.Ticker may be most appropriate. See Is there a way to do repetitive tasks at intervals in Golang?

Is the handling of concurrency with map correct?

Well, it's correct in the sense that you won't run into concurrency issues because you're accessing a map from multiple goroutines.

You may find sync.Map interesting (it's very new - came with Go 1.9).

That said, if you're using mutexes, I always prefer this pattern:

func lockingFunction(...) {
    mutex.Lock()
    defer mutex.Unlock()
    // do things
}

For a map, I'd usually create my own wrapper type around the map with Get and Set methods and then use that pattern inside to make it a synchronized map. Of course, sync.Map made this obsolete, but giving it just for example sake.

One problem in your code is that your checkUser function must read, check and (potentially) delete under the same lock. This can only be done with a mutex (but I'd still move that logic to a separate function that uses the lockedFunction pattern above).

Relative* scalability given use of synchronization primitive for map?

I have no good answer here. As mentioned above, I'd go with a single goroutine rather than one per user, which would be "more scalable". The mutex is certainly not doing you any favours, but it's hard to tell how bad this is at scale.

Do goroutines, specifically the "child" ones, exit upon returning or does exit-ing need to be done explicitly?

You don't need to (and can't) explicitly exit goroutines - when the function finishes (or panics), the goroutine is done.


A few other tips follow.


var mutex = &sync.Mutex{}

While this is perfectly valid, I prefer new in these situations:

var mutex = new(sync.Mutex)

It just looks cleaner. YMMV, of course.


A few of the things you use inline should be constants defined at the top:

  • 60 * time.Second
  • "Userid"
  • "static"
  • ":8080" (should probably be a flag)

Use log.Println in place of fmt.Println - all of your prints seem more appropriate as log entries. Also use fmt.Printf when printing "more complex" messages, for example:

    fmt.Println("Received registration for", userIDArray[0], "at", time.Now())

can be

    log.Printf("Received registration for %q at %v", userIDArray[0], time.Now())

    userIDArray, ok := r.Header["Userid"]
    fmt.Println("handleHeartbeat", time.Now())
    if ok {

You can simplify this:

    fmt.Println("handleHeartbeat", time.Now())
    if userID := r.Header.Get("Userid"); userID != "" {

That way, you don't need to use userIDArray[0].


        mutex.Lock()
        users[userIDArray[0]] = time.Now()
        mutex.Unlock()

Exact same block in two places - handleHeartbeat and handleRegister - not very DRY. While avoiding duplication at all costs is no good, this bit of code is technically shared between the two - on registration, the user automatically sends a heartbeat, so it should definitely be a separate function called by both handleHeartbeat and handleRegister (great candidate for the lockingFunction pattern above).

\$\endgroup\$
  • \$\begingroup\$ actually, it's not one goroutine per user, but potentially more than that (there is no check if a user is already registered, so a user spamming F5 will get lots of goroutines running, potentially DOSing the server). \$\endgroup\$ – hoffmale Oct 26 '17 at 9:35
  • \$\begingroup\$ Good point - edited - I think the threat of DoS attacks here is severe by design. \$\endgroup\$ – fstanis Oct 26 '17 at 10:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.