Skip to content

[BCI-3989][solana] - CR methods err when service unstarted #829

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
Farber98 wants to merge 29 commits into
base: develop
Choose a base branch
from
Draft

[BCI-3989][solana] - CR methods err when service unstarted #829

Changes from 6 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
e4b1032
add missing methods to testing interface
Farber98 Aug 22, 2024
72b155d
rename interface tester methods
Farber98 Aug 23, 2024
ae54542
remove unnecesary Start method
Farber98 Aug 26, 2024
826bfb9
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 Aug 29, 2024
07e8c77
close method impl
Farber98 Aug 29, 2024
06201dc
svc not started test in solana
Farber98 Aug 29, 2024
411032a
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 Aug 30, 2024
e666621
start service again so cleanup closes gracefully
Farber98 Aug 30, 2024
c14eed5
bump common ref
Farber98 Sep 3, 2024
a0b44bd
close ignoring error as core does. It might be already closed
Farber98 Sep 3, 2024
fc6cfe8
add start. address comment
Farber98 Sep 4, 2024
b15fafd
bump common
Farber98 Sep 4, 2024
0c335b0
merge develop
Farber98 Sep 4, 2024
6254cdb
bump common
Farber98 Sep 4, 2024
9769b7d
bump data streams for core integration tests
Farber98 Sep 4, 2024
048d13c
fix v2 dependencies
Farber98 Sep 4, 2024
c82beda
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 Sep 5, 2024
53f53b6
fix conflicts with chain components pr
Farber98 Sep 5, 2024
f629984
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 Sep 12, 2024
1cf0ccd
fix pr conflicts
Farber98 Sep 12, 2024
6d6b396
bump common version
Farber98 Sep 12, 2024
3f986a2
add flag to control if we return cr started
Farber98 Sep 15, 2024
1f37e8a
bump common
Farber98 Sep 15, 2024
79b4500
bump common
Farber98 Sep 16, 2024
cf4ce48
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 Sep 16, 2024
71454f3
make gomodtidy
Farber98 Sep 16, 2024
b7f19d9
refactor codec and chaincomponents ifaces
Farber98 Sep 16, 2024
099f8de
Merge branch 'develop' into BCI-3989-cr-methods-error-when-unstarted
Farber98 Sep 16, 2024
bc3a0f4
bump common
Farber98 Sep 16, 2024
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
12 changes: 11 additions & 1 deletion pkg/solana/chainreader/chain_reader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,9 @@ func (r *chainReaderInterfaceTester) GetChainReader(t *testing.T) types.Contract

require.NoError(t, svc.Start(context.Background()))
t.Cleanup(func() {
require.NoError(t, svc.Close())
if svc.Ready() == nil {
require.NoError(t, svc.Close())
}

@jmank88 jmank88 Aug 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These need to remain unchanged. It is never correct to skip Close() after calling Start, and this should be the only place that it happens.

Suggested change
if svc.Ready() == nil {
require.NoError(t, svc.Close())
}
require.NoError(t, svc.Close())

@Farber98 Farber98 Sep 3, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right! Doing it same way as core so we don't skip Close but can execute it a second time without failing hard a0b44bd (#829)

@jmank88 jmank88 Sep 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

but can execute it a second time without failing hard

What do you mean by this? It is incorrect by definition to call Close a second time. This must be the only location that calls Close, and it must check the returned error.

@Farber98 Farber98 Sep 3, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll take GetLatestValue as an example for code references.

I'm trying to test that calling the CR methods returns an err when the service is not Started. To achieve this, I'm calling the Close() method that was added to the testing interface "early". The method called in Solana is this one.

In Solana GetChainReader() is calling Start() and the cleanup() is calling Close(). When we run this test, we Close() the service for the first time and then when executing the cleanup(), we will get an error because we are trying to Close() again. That's why I removed the require.NoError() to do it the same way as it is in Core. By doing this, we are sure that the service was closed but we don't fail with the second Close() attempt.

The other thing I could do is remove the Start() and Close() calls inside the GetChainReader() in Solana and Core and execute them as needed in every test. It will introduce a lot of redundant calls to Start() and Close() but we will be able to control when we want to call these funcs avoiding double Start/Close problems.

Let me know your thoughts and if you have a better approach to achieve this

@jmank88 jmank88 Sep 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's why I removed the require.NoError() to do it the same way as it is in Core.

This existing code in incorrect. This is a bug.

By doing this, we are sure that the service was closed but we don't fail with the second Close() attempt.

Suppressing the error without checking it does not guarantee this. Other errors are possible than a double close error - but that still should not be ignored regardless. It is incorrect by definition to call Close a second time, and it is never necessary to do so.

The other thing I could do is remove the Start() and Close() calls inside the GetChainReader() in Solana and Core and execute them as needed in every test. It will introduce a lot of redundant calls to Start() and Close() but we will be able to control when we want to call these funcs avoiding double Start/Close problems.

I don't understand what you mean by this. Can you elaborate?

If the goal is to evaluate errors returned when not started, then all that is necessary is to make method calls either before starting or after closing. Is something preventing that from being done in a straightforward way?

@ilija42 ilija42 Sep 3, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The core code should be fixed, there is an error when Close() is called because the db instance gets closed before CR. This is happening becauseGetChainReader() calls into NewSqlxDB() which calls into t.Cleanup() before Setup() is called and calls t.Cleanup() which means that sqlx db gets cleaned up first.
We shouldn't ignore this error, but rather fix the cause

@Farber98 Farber98 Sep 4, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification!

The other thing I could do is remove the Start() and Close() calls inside the GetChainReader() in Solana and Core and execute them as needed in every test. It will introduce a lot of redundant calls to Start() and Close() but we will be able to control when we want to call these funcs avoiding double Start/Close problems.

I was thinking on exposing the Start() and Close() methods in the ChainReaderInterfaceTester and be able to call them from the tests in the moments we want rather than receiving it already started by the Setup() and having a mandatory Close() call at the end executed by the cleanup(). In this way we could call Start() and Close() when we need to and defer the Close() call if we want it to act as a cleanup()

@Farber98 Farber98 Sep 4, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Pushed these changes in the 3 repos. CI will fail until smartcontractkit/chainlink#14252 is merged and I rebase core. Tests were passing locally.

})

if r.reader == nil {
Expand All @@ -534,6 +536,10 @@ func (r *chainReaderInterfaceTester) GetChainReader(t *testing.T) types.Contract
return r.reader
}

func (r *chainReaderInterfaceTester) Close(t *testing.T) {
require.NoError(t, r.reader.service.Close())
}

type wrappedTestChainReader struct {
test *testing.T
service *chainreader.SolanaChainReaderService
Expand Down Expand Up @@ -621,6 +627,10 @@ func (r *wrappedTestChainReader) GetLatestValue(ctx context.Context, contractNam

fallthrough
default:
if r.service.Ready() != nil {
return errors.New("service not ready")
Comment thread
Farber98 marked this conversation as resolved.
Outdated
}

if len(r.testStructQueue) == 0 {
r.test.FailNow()
}
Expand Down