Repository: libp2p/go-libp2p
Pull request #1998 (2023-01-17 11:17:42)

Fix: Ignore zero values when marshalling Limits.
Ignoring zero values when marshaling zero values as it's done here https://github.com/libp2p/go-libp2p/blob/master/p2p/host/resource-manager/limit_defaults.go#L111

Outputs will change from:

```json
"Transient": {
      "Conns": 10,
      "ConnsInbound": 0,
      "ConnsOutbound": 0,
      "FD": 0,
      "Memory": 0,
      "Streams": 0,
      "StreamsInbound": 0,
      "StreamsOutbound": 0
}
```
to:

```json
"Transient": {
      "Conns": 10
}
```

We need to confirm what zero means for Resource Manager. My understanding after asking @marten-seemann is that zero means zero, not infinite or not set.

If zero means zero, we need to specify a way to define no limits for some of the fields inside a BaseLimit or a BaseLimitIncrease object. We can use `-1` or use pointers to allow `nil` values.

MarcoPolo: > We need to confirm what zero means for Resource Manager. My understanding after asking @marten-seemann is that zero means zero, not infinite or not set.

Zero means zero when inside the final struct used by go-libp2p. For example, when I start up a simple go-libp2p server and set `GOLOG_LOG_LEVEL=rcmgr=debug` I see the [limit config](https://gist.github.com/MarcoPolo/e84f825974964c660e16654696180efc) that go-libp2p uses. There are only zeros where it's unused. For example, Protocols have limits for the number of streams, but 0 for connections. They aren't consulted about number of connections since it's not really related to the protocol. In this case, it's still a zero value but it's just not used when checking to see if we can accept a connection or not.

When reading from JSON or [`Apply`ing](https://github.com/libp2p/go-libp2p/blob/master/p2p/host/resource-manager/limit.go#L102), zero means not set and it should use the default/other-value.

This is pretty confusing. And I think we should have a way of defining not-set vs zero that is uniform across serialization.

> If zero means zero, we need to specify a way to define no limits for some of the fields inside a BaseLimit or a BaseLimitIncrease object. We can use -1 or use pointers to allow nil values.

No limits can be MaxInt32 and that should work fine. Is there some reason to prefer a special value?
MarcoPolo: I think this change is pretty safe since it doesn't affect the semantics of how unmarshalled struct. What does this fix for you?

I'm thinking of adding a "IsSetFlags" bit field to `BaseLimit` and `BaseLimit` increase which will make it explicit if a field is unset and you would want a default override.

Patch:
diff --git a/p2p/host/resource-manager/limit.go b/p2p/host/resource-manager/limit.go
index 7d0823b1..c3d6dd88 100644
--- a/p2p/host/resource-manager/limit.go
+++ b/p2p/host/resource-manager/limit.go
@@ -87,14 +87,14 @@ func NewFixedLimiter(conf LimitConfig) Limiter {
 
 // BaseLimit is a mixin type for basic resource limits.
 type BaseLimit struct {
-	Streams         int
-	StreamsInbound  int
-	StreamsOutbound int
-	Conns           int
-	ConnsInbound    int
-	ConnsOutbound   int
-	FD              int
-	Memory          int64
+	Streams         int   `json:",omitempty"`
+	StreamsInbound  int   `json:",omitempty"`
+	StreamsOutbound int   `json:",omitempty"`
+	Conns           int   `json:",omitempty"`
+	ConnsInbound    int   `json:",omitempty"`
+	ConnsOutbound   int   `json:",omitempty"`
+	FD              int   `json:",omitempty"`
+	Memory          int64 `json:",omitempty"`
 }
 
 // Apply overwrites all zero-valued limits with the values of l2
@@ -128,16 +128,16 @@ func (l *BaseLimit) Apply(l2 BaseLimit) {
 
 // BaseLimitIncrease is the increase per GiB of allowed memory.
 type BaseLimitIncrease struct {
-	Streams         int
-	StreamsInbound  int
-	StreamsOutbound int
-	Conns           int
-	ConnsInbound    int
-	ConnsOutbound   int
+	Streams         int `json:",omitempty"`
+	StreamsInbound  int `json:",omitempty"`
+	StreamsOutbound int `json:",omitempty"`
+	Conns           int `json:",omitempty"`
+	ConnsInbound    int `json:",omitempty"`
+	ConnsOutbound   int `json:",omitempty"`
 	// Memory is in bytes. Values over 1>>30 (1GiB) don't make sense.
-	Memory int64
+	Memory int64 `json:",omitempty"`
 	// FDFraction is expected to be >= 0 and <= 1.
-	FDFraction float64
+	FDFraction float64 `json:",omitempty"`
 }
 
 // Apply overwrites all zero-valued limits with the values of l2


Test patch:
diff --git a/p2p/host/resource-manager/limit_test.go b/p2p/host/resource-manager/limit_test.go
index b063e05c..9070045b 100644
--- a/p2p/host/resource-manager/limit_test.go
+++ b/p2p/host/resource-manager/limit_test.go
@@ -1,6 +1,7 @@
 package rcmgr
 
 import (
+	"encoding/json"
 	"runtime"
 	"testing"
 
@@ -141,3 +142,21 @@ func TestReadmeExample(t *testing.T) {
 	require.Equal(t, 384, limitConf.System.Conns)
 	require.Equal(t, 1000, limitConf.System.FD)
 }
+
+func TestSerializeJSON(t *testing.T) {
+	bl := BaseLimit{
+		Streams: 10,
+	}
+
+	out, err := json.Marshal(bl)
+	require.NoError(t, err)
+	require.Equal(t, "{\"Streams\":10}", string(out))
+
+	bli := BaseLimitIncrease{
+		Streams: 10,
+	}
+
+	out, err = json.Marshal(bli)
+	require.NoError(t, err)
+	require.Equal(t, "{\"Streams\":10}", string(out))
+}