Skip to content

fix: close inner reader on ptr clean up #39050

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+19 −7
Draft

fix: close inner reader on ptr clean up #39050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion processor/geoipprocessor/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module github.com/open-telemetry/opentelemetry-collector-contrib/processor/geoipprocessor

go 1.23.0
go 1.24.0

require (
github.com/maxmind/MaxMind-DB v0.0.0-20240605211347-880f6b4b5eb6
Expand Down
5 changes: 5 additions & 0 deletions processor/geoipprocessor/integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package geoipprocessor

import (
"os"
"runtime"
"testing"

"go.opentelemetry.io/otel/attribute"
Expand All @@ -29,4 +30,8 @@ func TestProcessorWithMaxMind(t *testing.T) {
compareAllSignals(cfg, tt.goldenDir)(t)
})
}

// ensure resources have been clean up (no open files left)
// https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/38961
runtime.GC()
}
9 changes: 8 additions & 1 deletion processor/geoipprocessor/internal/provider/maxmindprovider/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"errors"
"fmt"
"net"
"runtime"

"github.com/oschwald/geoip2-golang"
"go.opentelemetry.io/otel/attribute"
Expand Down Expand Up @@ -39,7 +40,13 @@ func newMaxMindProvider(cfg *Config) (*maxMindProvider, error) {
return nil, fmt.Errorf("could not open geoip database: %w", err)
}

return &maxMindProvider{geoReader: geoReader, langCode: defaultLanguageCode}, nil
provider := &maxMindProvider{geoReader: geoReader, langCode: defaultLanguageCode}

runtime.AddCleanup(provider, func(geoReader *geoip2.Reader) {
_ = geoReader.Close()
}, provider.geoReader)
Comment on lines +45 to +47
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer not use leverage runtime package to correctly handle cleanup which should be done in a shutdown call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given components must have a Stop method makes sense to do the Close explicitly. I am about the change the PR, just testing out the new runtime method :)


return provider, nil
}

// Location implements provider.GeoIPProvider for MaxMind. If a non City database type is used or no metadata is found in the database, an error will be returned.
Expand Down
10 changes: 5 additions & 5 deletions processor/geoipprocessor/internal/provider/maxmindprovider/provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package maxmind
import (
"context"
"net"
"os"
"runtime"
"testing"

Expand All @@ -32,11 +31,8 @@ func TestInvalidNewProvider(t *testing.T) {

// TestProviderLocation asserts that the MaxMind provider adds the geo location data given an IP.
func TestProviderLocation(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/38961")
}
// temporal files will be removed during test cleanup
tmpDBfiles := testdata.GenerateLocalDB(t, "./testdata")
defer os.RemoveAll(tmpDBfiles)

t.Parallel()

Expand Down Expand Up @@ -114,4 +110,8 @@ func TestProviderLocation(t *testing.T) {
assert.True(t, tt.expectedAttributes.Equals(&actualAttributes))
})
}

// ensure resources have been clean up (no open files left)
// https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/38961
runtime.GC()
}
Loading