search

nats-io / nats-server

High-Performance server for NATS.io, the cloud and edge native messaging system.
https://nats.io
Apache License 2.0
15.92k stars 1.41k forks source link

Reusing Options struct leads to unexpected behavior #5465

Closed robinkb closed 5 months ago

robinkb commented 5 months ago

Observed behavior

I am starting embedded NATS Servers. If I reuse the same Options struct to initialize them (with NewServer) and to configure them at runtime (with ReloadOptions), I get strange behavior from the server. See the snippet below:

package nats

import (
    "fmt"
    "testing"
    "time"

    nats "github.com/nats-io/nats-server/v2/server"
)

func TestProofEnableJetstream(t *testing.T) {
    s1opts := makeOptions(t, "s1")
    s1, err := nats.NewServer(s1opts)
    if err != nil {
        t.Fatal(err)
    }
    s1.ConfigureLogger()
    s1.Start()

    s2opts := makeOptions(t, "s2")
    s2, err := nats.NewServer(s2opts)
    if err != nil {
        t.Fatal(err)
    }
    s2.ConfigureLogger()
    s2.Start()

    // Give NATS servers time to start
    time.Sleep(2 * time.Second)

    s1clusterAddr := s1.ClusterAddr()
    s2clusterAddr := s2.ClusterAddr()

    s1routes := nats.RoutesFromStr(fmt.Sprintf("nats://%s:%d", s2clusterAddr.IP.String(), s2clusterAddr.Port))
    s2routes := nats.RoutesFromStr(fmt.Sprintf("nats://%s:%d", s1clusterAddr.IP.String(), s1clusterAddr.Port))

    s1opts.Routes = s1routes
    s2opts.Routes = s2routes

    s1opts.JetStream = true
    s2opts.JetStream = true

    if err := s1.ReloadOptions(s1opts); err != nil {
        t.Fatal(err)
    }
    if err := s2.ReloadOptions(s2opts); err != nil {
        t.Fatal(err)
    }

    time.Sleep(30 * time.Second)
}

func makeOptions(t *testing.T, name string) *nats.Options {
    t.Helper()

    return &nats.Options{
        ServerName: name,
        Port:       -1,
        JetStream:  false,
        StoreDir:   t.TempDir(),
        Cluster: nats.ClusterOpts{
            Name: "testing",
            Host: "localhost",
            Port: -1,
        },
    }
}

This results in the following logs:

[109053] [INF] Starting nats-server
[109053] [INF]   Version:  2.11.0-dev
[109053] [INF]   Git:      [not set]
[109053] [INF]   Cluster:  testing
[109053] [INF]   Name:     s1
[109053] [INF]   ID:       NBSBDJYXBDSYHIMKQZHQPUVU2HR3SKKPSBSULLAAOTNTV4ODOCU75JE4
[109053] [INF] Listening for client connections on 0.0.0.0:38847
[109053] [INF] Server is ready
[109053] [INF] Cluster name is testing
[109053] [INF] Starting nats-server
[109053] [INF]   Version:  2.11.0-dev
[109053] [INF]   Git:      [not set]
[109053] [INF]   Cluster:  testing
[109053] [INF]   Name:     s2
[109053] [INF]   ID:       NAA4USDFQBFMY4KM37BAGDD4SSHP6HNLT27OI5HSFRHPR5TAMN3HEDDW
[109053] [INF] Listening for client connections on 0.0.0.0:33515
[109053] [INF] Listening for route connections on localhost:46083
[109053] [INF] Server is ready
[109053] [INF] Cluster name is testing
[109053] [INF] Listening for route connections on localhost:41541
[109053] [INF] Reloaded: authorization users
[109053] [INF] Reloaded: accounts
[109053] [INF] Reloaded: JetStream
[109053] [INF] Restarting JetStream
[109053] [INF] Starting JetStream
[109053] [INF]     _ ___ _____ ___ _____ ___ ___   _   __  __
[109053] [INF]  _ | | __|_   _/ __|_   _| _ \ __| /_\ |  \/  |
[109053] [INF] | || | _|  | | \__ \ | | |   / _| / _ \| |\/| |
[109053] [INF]  \__/|___| |_| |___/ |_| |_|_\___/_/ \_\_|  |_|
[109053] [INF] 
[109053] [INF]          https://docs.nats.io/jetstream
[109053] [INF] 
[109053] [INF] ---------------- JETSTREAM ----------------
[109053] [INF]   Max Memory:      47.02 GB
[109053] [INF]   Max Storage:     23.50 GB
[109053] [INF]   Store Directory: "/tmp/TestProofEnableJetstream3713534204/001/jetstream"
[109053] [INF] -------------------------------------------
[109053] [INF] Starting JetStream cluster
[109053] [INF] Creating JetStream metadata controller
[109053] [INF] JetStream cluster bootstrapping
[109053] [INF] Reloaded server configuration
[109053] [INF] Reloaded: authorization users
[109053] [INF] Reloaded: accounts
[109053] [INF] Reloaded: JetStream
[109053] [INF] Restarting JetStream
[109053] [INF] Starting JetStream
[109053] [INF]     _ ___ _____ ___ _____ ___ ___   _   __  __
[109053] [INF]  _ | | __|_   _/ __|_   _| _ \ __| /_\ |  \/  |
[109053] [INF] | || | _|  | | \__ \ | | |   / _| / _ \| |\/| |
[109053] [INF]  \__/|___| |_| |___/ |_| |_|_\___/_/ \_\_|  |_|
[109053] [INF] 
[109053] [INF]          https://docs.nats.io/jetstream
[109053] [INF] 
[109053] [INF] ---------------- JETSTREAM ----------------
[109053] [INF]   Max Memory:      47.02 GB
[109053] [INF]   Max Storage:     23.50 GB
[109053] [INF]   Store Directory: "/tmp/TestProofEnableJetstream3713534204/002/jetstream"
[109053] [INF] -------------------------------------------
[109053] [INF] Starting JetStream cluster
[109053] [INF] Creating JetStream metadata controller
[109053] [INF] JetStream cluster bootstrapping
[109053] [INF] Reloaded server configuration
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...
[109053] [WRN] Waiting for routing to be established...

If I clone the Options before passing them to NewServer and Server.ReloadOptions, it works as expected. JetStream is enabled and forms a cluster successfully.

So for example in the snippet, on line 13, I change s1opts to s1opts.Clone(), and so on.

Expected behavior

It was suggested to me in Slack that having to call Clone() to pass the Options is not expected behavior.

Server and client version

NATS Server v2.10/11

Host environment

Linux, AMD64

Steps to reproduce

See snippet above.

ripienaar commented 5 months ago

Since NewServer() takes a pointer to the struct you simply cant be sure it wont be modified. So do not reuse it.

Make new ones or deep copy a template one

robinkb commented 5 months ago

Fair. Does it make sense to add documentation? As you said, the fact that NewServer() takes a pointer already heavily implies that you shouldn't reuse. If not, I would just close the issue.

ripienaar commented 5 months ago

I am sure more documentation on these functions can only help :) Be great if you sent a PR