Add a refcount to dl load() and close() calls

[klueska]

No description provided.

[elezar]

I think this looks good. I thing our negative check and handling errors in this case need to be adjusted.

Added a suggestion for how to deal with this -- although I understand if we think it's a bit convoluted.

[elezar]

Question: Does something like:

defer l.refcount.IncOnNoError(rerror)

make sense here. (adjusting the l.refcount > 1 of course).

Also, are we assured that this defer would execute before the unlock?

[klueska]

I guess it depends on if we want to up the count before we do anything, or up the count after we've done everything.

Also, are we assured that this defer would execute before the unlock?

Yes. Defers are called last-in-first-out

[elezar]

What about when refcount == 0 to start with and then does not get decreased to -1 (because we're not allowing negative values). If there is then an error, our post-condition for calling close is then that refcount == 1 and not refcount == 0.

[klueska]

Yeah, I need to handle that.

[elezar]

What about:

Suggested change

	func (r *refcount) Dec() {
	if *r > 0 {
	(*r)--
	}
	}
	func (r *refcount) Dec() func(error) {
	if *r > 0 {
	(*r)--
	return r.IncOnError
	}
	// return a no-op if we don't decrease refcount.
	return func(error) {}
	}

And then we would use this:

defer l.refcount.Dec()(rerr)

?

[klueska]

This is less readable to me than just having two separate calls.

[elezar]

I meant that this handles the zero case, but sure.

[elezar]

When do we ever decrease the refcount then?

[elezar]

Thanks for this @klueska

Minor comments on testing top-level failure cases and using the mock properties instead of tracking calls explicitly.

[elezar]

I don't think this is required. You could use len(dl.calls.Open) and len(dl.calls.Close) directly.

[elezar]

diff --git a/pkg/nvml/lib_test.go b/pkg/nvml/lib_test.go
index a800b7e..ad5a01e 100644
--- a/pkg/nvml/lib_test.go
+++ b/pkg/nvml/lib_test.go
@@ -135,15 +135,11 @@ func TestLookupFromDefault(t *testing.T) {
 }
 
 func TestLoadAndCloseNesting(t *testing.T) {
-	var numOpens, numCloses int
-
 	dl := &dynamicLibraryMock{
 		OpenFunc: func() error {
-			numOpens++
 			return nil
 		},
 		CloseFunc: func() error {
-			numCloses++
 			return nil
 		},
 	}
@@ -153,22 +149,22 @@ func TestLoadAndCloseNesting(t *testing.T) {
 	GetLibrary()
 
 	// When calling load twice, the library was only opened once
-	require.Equal(t, 0, numOpens)
+	require.Equal(t, 0, len(dl.calls.Open))
 	require.Nil(t, libnvml.load())
-	require.Equal(t, 1, numOpens)
+	require.Equal(t, 1, len(dl.calls.Open))
 	require.Nil(t, libnvml.load())
-	require.Equal(t, 1, numOpens)
+	require.Equal(t, 1, len(dl.calls.Open))
 
 	// Only after calling close twice, was the library closed
-	require.Equal(t, 0, numCloses)
+	require.Equal(t, 0, len(dl.calls.Close))
 	require.Nil(t, libnvml.close())
-	require.Equal(t, 0, numCloses)
+	require.Equal(t, 0, len(dl.calls.Close))
 	require.Nil(t, libnvml.close())
-	require.Equal(t, 1, numCloses)
+	require.Equal(t, 1, len(dl.calls.Close))
 
 	// Calling close again doesn't attempt to close the library again
 	require.Nil(t, libnvml.close())
-	require.Equal(t, 1, numCloses)
+	require.Equal(t, 1, len(dl.calls.Close))
 }
 
 func setNewDynamicLibraryDuringTest(dl dynamicLibrary) func() {

[klueska]

Integrating...

[elezar]

Is the GetLibrary() call required?

[klueska]

I thought I did, but you're right, it appears I don't. Removing it...

[elezar]

Could we add the following initial check too:

	// When calling close before opening the library nothing happens.
	require.Equal(t, 0, len(dl.calls.Close))
	require.Nil(t, libnvml.close())
	require.Equal(t, 0, len(dl.calls.Close))

to ensure that we don't try to close a nil library?

[klueska]

Integrating ...

[elezar]

We are missing test cases for when dl.Open or dl.Close returns errors.

[elezar]

I added the following to cover the basics.

diff --git a/pkg/nvml/lib_test.go b/pkg/nvml/lib_test.go
index a800b7e..ce30111 100644
--- a/pkg/nvml/lib_test.go
+++ b/pkg/nvml/lib_test.go
@@ -171,6 +171,80 @@ func TestLoadAndCloseNesting(t *testing.T) {
 	require.Equal(t, 1, numCloses)
 }
 
+func TestLoadAndCloseWithErrors(t *testing.T) {
+	testCases := []struct {
+		description           string
+		dl                    dynamicLibrary
+		expectedLoadRefcount  refcount
+		expectedCloseRefcount refcount
+	}{
+		{
+			description: "regular flow",
+			dl: &dynamicLibraryMock{
+				OpenFunc: func() error {
+					return nil
+				},
+				CloseFunc: func() error {
+					return nil
+				},
+			},
+			expectedLoadRefcount:  1,
+			expectedCloseRefcount: 0,
+		},
+		{
+			description: "open error",
+			dl: &dynamicLibraryMock{
+				OpenFunc: func() error {
+					return errors.New("")
+				},
+				CloseFunc: func() error {
+					return nil
+				},
+			},
+			expectedLoadRefcount:  0,
+			expectedCloseRefcount: 0,
+		},
+		{
+			description: "close error",
+			dl: &dynamicLibraryMock{
+				OpenFunc: func() error {
+					return nil
+				},
+				CloseFunc: func() error {
+					return errors.New("")
+				},
+			},
+			expectedLoadRefcount:  1,
+			expectedCloseRefcount: 1,
+		},
+		{
+			description: "open and close error",
+			dl: &dynamicLibraryMock{
+				OpenFunc: func() error {
+					return errors.New("")
+				},
+				CloseFunc: func() error {
+					return errors.New("")
+				},
+			},
+			expectedLoadRefcount:  0,
+			expectedCloseRefcount: 0,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.description, func(t *testing.T) {
+			defer setNewDynamicLibraryDuringTest(tc.dl)()
+			defer resetLibrary()
+
+			_ = libnvml.load()
+			require.Equal(t, tc.expectedLoadRefcount, libnvml.refcount)
+			_ = libnvml.close()
+			require.Equal(t, tc.expectedCloseRefcount, libnvml.refcount)
+		})
+	}
+}
+
 func setNewDynamicLibraryDuringTest(dl dynamicLibrary) func() {
 	original := newDynamicLibrary
 	newDynamicLibrary = func(string, int) dynamicLibrary {

[klueska]

Integrating...

[elezar]

Just to clarify, I think the additions that I proposed are fine as follow-ups.