From 5af4b72492cb200f73d8310d0b9799f7bb9721ca Mon Sep 17 00:00:00 2001 From: Noam Berg Date: Sun, 27 Dec 2020 19:39:27 +0200 Subject: [PATCH 1/3] change timeouts in config, add timeout for the http get in the mgmt-file-provider --- config/config_system_presets.go | 4 ++-- services/management/adapter/file_provider.go | 10 ++++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/config/config_system_presets.go b/config/config_system_presets.go index 7cb896215..4b72079c4 100644 --- a/config/config_system_presets.go +++ b/config/config_system_presets.go @@ -19,8 +19,8 @@ func defaultProductionConfig() mutableNodeConfig { cfg := emptyConfig() cfg.SetUint32(GOSSIP_LISTEN_PORT, 4400) - cfg.SetDuration(MANAGEMENT_POLLING_INTERVAL, 10*time.Second) - cfg.SetUint32(MANAGEMENT_MAX_FILE_SIZE, 50*(1<<20)) // 50 MB + cfg.SetDuration(MANAGEMENT_POLLING_INTERVAL, 30*time.Second) + cfg.SetUint32(MANAGEMENT_MAX_FILE_SIZE, 500*(1<<20)) // 50 MB cfg.SetDuration(MANAGEMENT_CONSENSUS_GRACE_TIMEOUT, 10*time.Minute) // for private consider changing this to 2^62 nanos (100 years) for PoS v2 cfg.SetDuration(COMMITTEE_GRACE_PERIOD, 12*time.Hour) diff --git a/services/management/adapter/file_provider.go b/services/management/adapter/file_provider.go index 19e1f0542..0f86503ed 100644 --- a/services/management/adapter/file_provider.go +++ b/services/management/adapter/file_provider.go @@ -23,6 +23,7 @@ import ( "os" "sort" "strings" + "time" ) type FileConfig interface { @@ -34,10 +35,14 @@ type FileConfig interface { type FileProvider struct { logger log.Logger config FileConfig + client *http.Client } func NewFileProvider(config FileConfig, logger log.Logger) *FileProvider { - return &FileProvider{config: config, logger: logger} + client := &http.Client{ + Timeout: 45 * time.Second, + } + return &FileProvider{config: config, logger: logger, client:client} } func (mp *FileProvider) Get(ctx context.Context, referenceTime primitives.TimestampSeconds) (*management.VirtualChainManagementData, error) { @@ -78,7 +83,8 @@ func (mp *FileProvider) generatePath(referenceTime primitives.TimestampSeconds) } func (mp *FileProvider) readUrl(path string) ([]byte, error) { - res, err := http.Get(path) + res, err := mp.client.Get(path) + defer res.Body.Close() if err != nil || res == nil { return nil, errors.Wrapf(err, "Failed http get of url %s", path) From 83b6d78e15bfb5ecd0069e0cbceb212af7f96797 Mon Sep 17 00:00:00 2001 From: Noam Berg Date: Mon, 28 Dec 2020 10:59:47 +0200 Subject: [PATCH 2/3] change close to fit go guidelines, remove un-needed logs inside file-provider. --- bootstrap/node.go | 2 +- services/management/adapter/file_provider.go | 24 +++++++++---------- .../management/adapter/file_provider_test.go | 10 ++++---- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/bootstrap/node.go b/bootstrap/node.go index c97ff680c..381df2a63 100644 --- a/bootstrap/node.go +++ b/bootstrap/node.go @@ -68,7 +68,7 @@ func NewNode(nodeConfig config.NodeConfig, logger log.Logger) *Node { nodeLogger.Error("Cannot start node without a ManagementFilePath", log.Error(err)) panic(err) } else { - managementProvider = managementAdapter.NewFileProvider(nodeConfig, nodeLogger) + managementProvider = managementAdapter.NewFileProvider(nodeConfig) } blockPersistence, err := filesystem.NewBlockPersistence(nodeConfig, nodeLogger, metricRegistry) diff --git a/services/management/adapter/file_provider.go b/services/management/adapter/file_provider.go index 0f86503ed..e7c83b6bc 100644 --- a/services/management/adapter/file_provider.go +++ b/services/management/adapter/file_provider.go @@ -16,7 +16,6 @@ import ( "github.com/orbs-network/orbs-network-go/services/management" "github.com/orbs-network/orbs-spec/types/go/primitives" "github.com/orbs-network/orbs-spec/types/go/services" - "github.com/orbs-network/scribe/log" "github.com/pkg/errors" "io/ioutil" "net/http" @@ -33,16 +32,15 @@ type FileConfig interface { } type FileProvider struct { - logger log.Logger config FileConfig client *http.Client } -func NewFileProvider(config FileConfig, logger log.Logger) *FileProvider { +func NewFileProvider(config FileConfig) *FileProvider { client := &http.Client{ Timeout: 45 * time.Second, } - return &FileProvider{config: config, logger: logger, client:client} + return &FileProvider{config: config, client:client} } func (mp *FileProvider) Get(ctx context.Context, referenceTime primitives.TimestampSeconds) (*management.VirtualChainManagementData, error) { @@ -52,21 +50,18 @@ func (mp *FileProvider) Get(ctx context.Context, referenceTime primitives.Timest if strings.HasPrefix(path, "http") { if contents, err = mp.readUrl(path); err != nil { - mp.logger.Error("Provider url reading error", log.Error(err)) - return nil, err + return nil, errors.Wrap(err, "Provider url reading error") } } else { if contents, err = mp.readFile(path); err != nil { - mp.logger.Error("Provider path file reading error", log.Error(err)) - return nil, err + return nil, errors.Wrap(err, "Provider path file reading error") } } isHistoric := referenceTime != 0 managementData, parseErr := mp.parseData(contents, isHistoric) if parseErr != nil { - mp.logger.Error("Provider file parsing error", log.Error(parseErr)) - return nil, parseErr + return nil, errors.Wrap(err, "Provider file parsing error") } return managementData, nil @@ -84,7 +79,6 @@ func (mp *FileProvider) generatePath(referenceTime primitives.TimestampSeconds) func (mp *FileProvider) readUrl(path string) ([]byte, error) { res, err := mp.client.Get(path) - defer res.Body.Close() if err != nil || res == nil { return nil, errors.Wrapf(err, "Failed http get of url %s", path) @@ -92,7 +86,13 @@ func (mp *FileProvider) readUrl(path string) ([]byte, error) { return nil, errors.Wrapf(err, "Failed http get response too big %d", res.ContentLength) } - return ioutil.ReadAll(res.Body) + contents, err := ioutil.ReadAll(res.Body) + if err != nil { + return nil, errors.Wrapf(err, "could not read http body") + } + defer res.Body.Close() + + return contents, err } func (mp *FileProvider) readFile(filePath string) ([]byte, error) { diff --git a/services/management/adapter/file_provider_test.go b/services/management/adapter/file_provider_test.go index f5bd261e5..43adf8b4b 100644 --- a/services/management/adapter/file_provider_test.go +++ b/services/management/adapter/file_provider_test.go @@ -23,7 +23,7 @@ func TestManagementFileProvider_GeneratePath(t *testing.T) { with.Logging(t, func(parent *with.LoggingHarness) { const url = "x1" cfg := newConfig(42, url) - fileProvider := NewFileProvider(cfg, parent.Logger) + fileProvider := NewFileProvider(cfg) path := fileProvider.generatePath(0) pathWithRef := fileProvider.generatePath(100) require.Equal(t, url, path) @@ -35,7 +35,7 @@ func TestManagementFileProvider_NoMatchVc(t *testing.T) { with.Logging(t, func(parent *with.LoggingHarness) { with.Context(func(ctx context.Context) { cfg := newConfig(42, "") - fileProvider := NewFileProvider(cfg, parent.Logger) + fileProvider := NewFileProvider(cfg) _, err := fileProvider.parseData([]byte(`{ "CurrentRefTime": 3, "PageStartRefTime": 0, @@ -55,7 +55,7 @@ func TestManagementFileProvider_BadCurrentInPage(t *testing.T) { with.Logging(t, func(parent *with.LoggingHarness) { with.Context(func(ctx context.Context) { cfg := newConfig(42, "") - fileProvider := NewFileProvider(cfg, parent.Logger) + fileProvider := NewFileProvider(cfg) _, err := fileProvider.parseData([]byte(`{ "CurrentRefTime": 3, "PageStartRefTime": 0, @@ -112,7 +112,7 @@ func TestManagementFileProvider_ReadFile(t *testing.T) { with.Logging(t, func(parent *with.LoggingHarness) { topologyFilePath := filepath.Join(config.GetCurrentSourceFileDirPath(), "_data", "good.json") cfg := newConfig(42, topologyFilePath) - fileProvider := NewFileProvider(cfg, parent.Logger) + fileProvider := NewFileProvider(cfg) expectFileProviderToReadCorrectly(t, ctx, fileProvider) }) }) @@ -123,7 +123,7 @@ func TestManagementFileProvider_ReadUrl(t *testing.T) { with.Logging(t, func(parent *with.LoggingHarness) { const url = "https://raw.githubusercontent.com/orbs-network/management-service/master/static-tests/management-vc-file.json" cfg := newConfig(42, url) - fileProvider := NewFileProvider(cfg, parent.Logger) + fileProvider := NewFileProvider(cfg) expectFileProviderToReadCorrectly(t, ctx, fileProvider) }) }) From 31629ef5b353bc53193d5bd0f23bbd140e8049fd Mon Sep 17 00:00:00 2001 From: Noam Berg Date: Tue, 29 Dec 2020 11:34:15 +0200 Subject: [PATCH 3/3] defer if res and body not nil (even if there is err) --- go.mod | 1 - services/management/adapter/file_provider.go | 11 ++++------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/go.mod b/go.mod index 3fe7c3e07..8b4493ef0 100644 --- a/go.mod +++ b/go.mod @@ -17,7 +17,6 @@ require ( github.com/ethereum/go-ethereum v1.9.6 github.com/fjl/memsize v0.0.0-20190710130421-bcb5799ab5e5 // indirect github.com/gballet/go-libpcsclite v0.0.0-20190607065134-2772fd86a8ff // indirect - github.com/go-stack/stack v1.8.0 // indirect github.com/google/go-cmp v0.3.1 github.com/huin/goupnp v1.0.0 // indirect github.com/jackpal/go-nat-pmp v1.0.1 // indirect diff --git a/services/management/adapter/file_provider.go b/services/management/adapter/file_provider.go index e7c83b6bc..08c905b67 100644 --- a/services/management/adapter/file_provider.go +++ b/services/management/adapter/file_provider.go @@ -79,6 +79,9 @@ func (mp *FileProvider) generatePath(referenceTime primitives.TimestampSeconds) func (mp *FileProvider) readUrl(path string) ([]byte, error) { res, err := mp.client.Get(path) + if res != nil && res.Body != nil { + defer res.Body.Close() + } if err != nil || res == nil { return nil, errors.Wrapf(err, "Failed http get of url %s", path) @@ -86,13 +89,7 @@ func (mp *FileProvider) readUrl(path string) ([]byte, error) { return nil, errors.Wrapf(err, "Failed http get response too big %d", res.ContentLength) } - contents, err := ioutil.ReadAll(res.Body) - if err != nil { - return nil, errors.Wrapf(err, "could not read http body") - } - defer res.Body.Close() - - return contents, err + return ioutil.ReadAll(res.Body) } func (mp *FileProvider) readFile(filePath string) ([]byte, error) {