Merge pull request #4 from kolesa-team/get-bytes-fix

пробуем отловить проблему

Author: antonsergeyev

.travis.yml

@@ -1,5 +1,5 @@
 language: go
-go: 1.13
+go: 1.18
 go_import_path: https://github.com/kolesa-team/goexiv
 
 cache:
@@ -22,7 +22,7 @@ before_install:
     test -d exiv2-${EXIV2_VERSION} && {
       cd exiv2-${EXIV2_VERSION}/build
     } || {
-      wget http://www.exiv2.org/builds/exiv2-${EXIV2_VERSION}-Source.tar.gz
+      wget https://github.com/Exiv2/exiv2/releases/download/v${EXIV2_VERSION}/exiv2-${EXIV2_VERSION}-Source.tar.gz
       tar xzf exiv2-${EXIV2_VERSION}-Source.tar.gz
       mv exiv2-${EXIV2_VERSION}-Source exiv2-${EXIV2_VERSION}
       cd exiv2-${EXIV2_VERSION}

README.md

@@ -71,6 +71,10 @@ if err != nil {
 
 fmt.Println(userComment)
 // "A comment. Might be a JSON string. Можно писать и по-русски!"
+
+// It is advisable to insert this line at the end of `goexivImg` lifecycle.  
+// see exiv_test.go:Test_GetBytes_Goroutine
+runtime.KeepAlive(goexivImg)
 ```
 
 Changing the image metadata in memory and returning the updated image (an approach fit for a web service):

exiv.go

@@ -7,7 +7,6 @@ import "C"
 
 import (
 	"errors"
-	"reflect"
 	"runtime"
 	"unsafe"
 )
@@ -18,7 +17,8 @@ type Error struct {
 }
 
 type Image struct {
-	img *C.Exiv2Image
+	bytesArrayPtr unsafe.Pointer
+	img           *C.Exiv2Image
 }
 
 type MetadataProvider interface {
@@ -42,13 +42,18 @@ func makeError(cerr *C.Exiv2Error) *Error {
 	}
 }
 
-func makeImage(cimg *C.Exiv2Image) *Image {
+func makeImage(cimg *C.Exiv2Image, bytesPtr unsafe.Pointer) *Image {
 	img := &Image{
-		cimg,
+		bytesArrayPtr: bytesPtr,
+		img:           cimg,
 	}
 
 	runtime.SetFinalizer(img, func(x *Image) {
 		C.exiv2_image_free(x.img)
+
+		if x.bytesArrayPtr != nil {
+			C.free(x.bytesArrayPtr)
+		}
 	})
 
 	return img
@@ -71,26 +76,33 @@ func Open(path string) (*Image, error) {
 		return nil, err
 	}
 
-	return makeImage(cimg), nil
+	return makeImage(cimg, nil), nil
 }
 
 // OpenBytes opens a byte slice with image data and returns a pointer to
 // the corresponding Image object, but does not read the Metadata.
 // Start the parsing with a call to ReadMetadata()
-func OpenBytes(b []byte) (*Image, error) {
-	if len(b) == 0 {
+func OpenBytes(input []byte) (*Image, error) {
+	if len(input) == 0 {
 		return nil, &Error{0, "input is empty"}
 	}
+
 	var cerr *C.Exiv2Error
-	cimg := C.exiv2_image_factory_open_bytes((*C.uchar)(unsafe.Pointer(&b[0])), C.long(len(b)), &cerr)
+
+	bytesArrayPtr := C.CBytes(input)
+	cimg := C.exiv2_image_factory_open_bytes(
+		(*C.uchar)(bytesArrayPtr),
+		C.long(len(input)),
+		&cerr,
+	)
 
 	if cerr != nil {
 		err := makeError(cerr)
 		C.exiv2_error_free(cerr)
 		return nil, err
 	}
 
-	return makeImage(cimg), nil
+	return makeImage(cimg, bytesArrayPtr), nil
 }
 
 // ReadMetadata reads the metadata of an Image
@@ -111,19 +123,10 @@ func (i *Image) ReadMetadata() error {
 // Returns an image contents.
 // If its metadata has been changed, the changes are reflected here.
 func (i *Image) GetBytes() []byte {
-	size := int(C.exiv_image_get_size(i.img))
+	size := C.exiv_image_get_size(i.img)
 	ptr := C.exiv_image_get_bytes_ptr(i.img)
 
-	var slice []byte
-	header := (*reflect.SliceHeader)(unsafe.Pointer(&slice))
-	header.Cap = size
-	header.Len = size
-	header.Data = uintptr(unsafe.Pointer(ptr))
-
-	target := make([]byte, len(slice))
-	copy(target, slice)
-
-	return target
+	return C.GoBytes(unsafe.Pointer(ptr), C.int(size))
 }
 
 // PixelWidth returns the width of the image in pixels

exiv_test.go

@@ -5,6 +5,8 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"io/ioutil"
+	"runtime"
+	"sync"
 	"testing"
 )
 
@@ -407,6 +409,90 @@ func Test_GetBytes(t *testing.T) {
 	)
 }
 
+// Ensures image manipulation doesn't fail when running from multiple goroutines
+func Test_GetBytes_Goroutine(t *testing.T) {
+	var wg sync.WaitGroup
+	iterations := 0
+
+	bytes, err := ioutil.ReadFile("testdata/stripped_pixel.jpg")
+	require.NoError(t, err)
+
+	for i := 0; i < 100; i++ {
+		iterations++
+		wg.Add(1)
+
+		go func(i int) {
+			defer wg.Done()
+
+			img, err := goexiv.OpenBytes(bytes)
+			require.NoError(t, err)
+
+			// trigger garbage collection to increase the chance that underlying img.img will be collected
+			runtime.GC()
+
+			bytesAfter := img.GetBytes()
+			assert.NotEmpty(t, bytesAfter)
+
+			// if this line is removed, then the test will likely fail
+			// with segmentation violation.
+			// so far we couldn't come up with a better solution.
+			runtime.KeepAlive(img)
+		}(i)
+	}
+
+	wg.Wait()
+	runtime.GC()
+	var memStats runtime.MemStats
+	runtime.ReadMemStats(&memStats)
+	t.Logf("Allocated bytes after test:  %+v\n", memStats.HeapAlloc)
+}
+
+func BenchmarkImage_GetBytes_KeepAlive(b *testing.B) {
+	bytes, err := ioutil.ReadFile("testdata/stripped_pixel.jpg")
+	require.NoError(b, err)
+	var wg sync.WaitGroup
+
+	for i := 0; i < b.N; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+
+			img, err := goexiv.OpenBytes(bytes)
+			require.NoError(b, err)
+
+			runtime.GC()
+
+			require.NoError(b, img.SetExifString("Exif.Photo.UserComment", "123"))
+
+			bytesAfter := img.GetBytes()
+			assert.NotEmpty(b, bytesAfter)
+			runtime.KeepAlive(img)
+		}()
+	}
+
+	wg.Wait()
+}
+
+func BenchmarkImage_GetBytes_NoKeepAlive(b *testing.B) {
+	bytes, err := ioutil.ReadFile("testdata/stripped_pixel.jpg")
+	require.NoError(b, err)
+	var wg sync.WaitGroup
+
+	for i := 0; i < b.N; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			img, err := goexiv.OpenBytes(bytes)
+			require.NoError(b, err)
+
+			require.NoError(b, img.SetExifString("Exif.Photo.UserComment", "123"))
+
+			bytesAfter := img.GetBytes()
+			assert.NotEmpty(b, bytesAfter)
+		}()
+	}
+}
+
 // Fills the image with metadata
 func initializeImage(path string, t *testing.T) {
 	img, err := goexiv.Open(path)

go.mod

@@ -6,4 +6,4 @@ require (
 	github.com/stretchr/testify v1.2.2
 )
 
-go 1.13
+go 1.18