[#147] Add context to waiter #148

vdomnich-yadro · 2025-02-27T17:28:11Z

vdomnich-yadro commented

2025-02-27 17:28:11 +00:00

Adds new method WaitCtx that takes a context as a parameter. Application can cancel this context to terminate transaction awaiting.

Adds new method `WaitCtx` that takes a context as a parameter. Application can cancel this context to terminate transaction awaiting.

vdomnich-yadro added 1 commit 2025-02-27 17:28:11 +00:00

[#147 ] Add context to waiter

DCO action / DCO (pull_request) Successful in 30s

Details

Code generation / Generate wrappers (pull_request) Successful in 53s

Details

Tests / Tests (pull_request) Successful in 1m5s

Details

88232efe67

Signed-off-by: Vladimir Domnich <v.domnich@yadro.com>

requested reviews from dkirillov, fyrchik, storage-core-developers, storage-core-committers

2025-02-27 17:28:11 +00:00

fyrchik reviewed 2025-02-27 18:10:48 +00:00

commonclient/waiter.go Outdated

					
				@ -13,6 +13,16 @@ import (

				const alreadyExistsError = "already exists"

				// ContextWaiter is an interface for wrapper around [waiter.Waiter] that respects

fyrchik commented

2025-02-27 17:53:41 +00:00

It is not necessarily a wrapper, it is an interface, after all.

It is not necessarily a wrapper, it is an _interface_, after all.

vdomnich-yadro commented

2025-02-28 07:09:10 +00:00

Re-phrased the comment.

fyrchik marked this conversation as resolved

commonclient/waiter.go Outdated

					
				@ -16,0 +16,4 @@

				// ContextWaiter is an interface for wrapper around [waiter.Waiter] that respects

				// a context while waiting for a transaction.

				type ContextWaiter interface {

					// Deprecated: use WaitCtx instead to prevent waiter hang up when discrete time stops ticking.

fyrchik commented

2025-02-27 17:52:23 +00:00

Wait() and WaitAny() could be combined in waiter.Waiter

// ContextWaiter is waiter.Waiter that has an additional method for canceling wait by context.
type ContextWaiter interface {
   waiter.Waiter
   WaitCtx(...)

`Wait()` and `WaitAny()` could be combined in [waiter.Waiter](https://github.com/nspcc-dev/neo-go/blob/68d7e8e01cd8d4e5c8f35bb05949c5dc4718b18e/pkg/rpcclient/waiter/waiter.go#L41) ``` // ContextWaiter is waiter.Waiter that has an additional method for canceling wait by context. type ContextWaiter interface { waiter.Waiter WaitCtx(...) ```

vdomnich-yadro commented

2025-02-28 07:06:21 +00:00

Yep. I didn't do this in the first place in order to place the deprecation mark.

As it seems you would prefer to not have the deprecation mark, I will just embed the Waiter interface here, np.

Yep. I didn't do this in the first place in order to place the deprecation mark. As it seems you would prefer to not have the deprecation mark, I will just embed the Waiter interface here, np.

fyrchik marked this conversation as resolved

commonclient/waiter.go Outdated

					
				@ -44,6 +54,7 @@ func NewWaiter(waiter waiter.Waiter, options WaiterOptions) *Waiter {

				}

				// Wait allows to wait until transaction will be accepted to the chain.

				// Deprecated: use WaitCtx instead to prevent waiter hang up when discrete time stops ticking.

fyrchik commented

2025-02-27 17:52:54 +00:00

I don't see it being deprecated -- we will have both method implemented to retain composability with neo-go utility functions.

vdomnich-yadro commented

2025-02-28 07:03:43 +00:00

I've put Deprecated mark here in order to discourage the usage of this method. The deprecation mark raises warnings in the code where we use this method.

I am fine to remove the deprecation here if that is your preference. I have no strong feelings about it at all.

I've put Deprecated mark here in order to discourage the usage of this method. The deprecation mark raises warnings in the code where we use this method. I am fine to remove the deprecation here if that is your preference. I have no strong feelings about it at all.

fyrchik marked this conversation as resolved

commonclient/waiter.go Outdated

					
				@ -54,1 +65,4 @@

				// WaitCtx allows to wait until transaction is accepted to the chain.

				// Waiting is limited by the specified context.

				func (w *Waiter) WaitCtx(ctx context.Context, h util.Uint256, vub uint32, err error) (*state.AppExecResult, error) {

fyrchik commented

2025-02-27 17:59:35 +00:00

Wait() has 3 parameters to be easily composable with actor methods, so that we could write w.Wait(a.SignAndSend(tx)) succinctly.

WIth this signature we no longer have this property, so I am now wondering whether it is still useful to have an error here (I don't see the code you have)?
E.g. in morph we have

func (c *Client) WaitTxHalt(ctx context.Context, vub uint32, h util.Uint256) error {

`Wait()` has 3 parameters to be easily composable with [actor methods](https://github.com/nspcc-dev/neo-go/blob/68d7e8e01cd8d4e5c8f35bb05949c5dc4718b18e/pkg/rpcclient/actor/actor.go#L248), so that we could write `w.Wait(a.SignAndSend(tx))` succinctly. WIth this signature we no longer have this property, so I am now wondering whether it is still useful to have an error here (I don't see the code you have)? E.g. in morph we have https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/2ff032db90de1c1c2af24a9231eeafaabbe5d643/pkg/morph/client/waiter.go#L36

vdomnich-yadro commented

2025-02-28 07:02:01 +00:00

Yes, it is still useful to have an error as an extra parameter. In some implementations of waiters we have error conversion logic (that parses blockchain error text and transforms it into a Go error). It is convenient to pass both "tx send error" and "tx wait error" to waiter so that it applies the conversion logic.

fyrchik marked this conversation as resolved

vdomnich-yadro force-pushed vd/wait-ctx from 88232efe67 to 51bccdbcfb

2025-02-28 07:08:24 +00:00

Compare

vdomnich-yadro force-pushed vd/wait-ctx from 51bccdbcfb to 01a724d449

2025-02-28 07:09:57 +00:00

Compare

vdomnich-yadro was assigned by dkirillov

2025-02-28 08:17:23 +00:00

dkirillov approved these changes 2025-02-28 08:21:29 +00:00

dkirillov left a comment

LGTM

achuprov approved these changes 2025-02-28 09:43:37 +00:00

fyrchik reviewed 2025-02-28 10:25:26 +00:00

commonclient/waiter_test.go

					
				@ -71,3 +69,3 @@

						sendErr := fmt.Errorf("transaction already exists")

						mw := &mockWaiter{}

						mw.On("Wait", txHash, vub, mock.Anything).Return(mw.successfulResult(txHash), nil)

						mw.On("WaitAny", mock.Anything, vub, txHashes).Return(mw.successfulResult(txHash), nil)

fyrchik commented

2025-02-28 10:25:26 +00:00

What is the goal of changing all the tests?
Do we still test Wait() after all these changes?

IMO it would be better to just write tests on WaitCtx() in this PR, specifically that it calls WaitAny() inside.

What is the goal of changing all the tests? Do we still test `Wait()` after all these changes? IMO it would be better to just write tests on `WaitCtx()` in this PR, _specifically_ that it calls `WaitAny()` inside.

vdomnich-yadro commented

2025-02-28 15:18:46 +00:00

Do we still test Wait() after all these changes?

No, we don't.

IMO it would be better to just write tests on WaitCtx() in this PR, specifically that it calls WaitAny() inside.

If I get your proposal correctly, you would like to keep tests on Wait intact and add new tests to cover WaitCtx, right?
If so, sure, will do. Thanks!

> Do we still test Wait() after all these changes? No, we don't. > IMO it would be better to just write tests on WaitCtx() in this PR, specifically that it calls WaitAny() inside. If I get your proposal correctly, you would like to keep tests on `Wait` intact and add new tests to cover `WaitCtx`, right? If so, sure, will do. Thanks!

👍 1

fyrchik approved these changes 2025-02-28 10:25:36 +00:00

fyrchik left a comment

Other than the comment, LGTM

acid-ant reviewed 2025-03-05 07:29:42 +00:00

commonclient/waiter.go

					
				@ -55,0 +61,4 @@

				// WaitCtx allows to wait until transaction is accepted to the chain.

				// Waiting is limited by the specified context.

				func (w *Waiter) WaitCtx(ctx context.Context, h util.Uint256, vub uint32, err error) (*state.AppExecResult, error) {

					if err != nil {

acid-ant commented

2025-03-05 07:27:50 +00:00

Why not to use one if as in Waiter.Wait(...)?

if !w.options.IgnoreAlreadyExistsError && errIsAlreadyExists(err) {
	return nil, err
}

Why not to use one `if` as in `Waiter.Wait(...)`? ``` if !w.options.IgnoreAlreadyExistsError && errIsAlreadyExists(err) { return nil, err } ```

fyrchik commented

2025-03-05 07:44:52 +00:00

Because if !errIsAlreadyExists(err) && err != nil, the error will be skipped in your case.
In Wait() that is handled by Wait() method of the inner struct.

Because if `!errIsAlreadyExists(err) && err != nil`, the error will be skipped in your case. In `Wait()` that is handled by `Wait()` method of the inner struct.

acid-ant commented

2025-03-05 08:32:10 +00:00

Right, if should be a bit different:

if err != nil && !(errIsAlreadyExists(err) && w.options.IgnoreAlreadyExistsError) {
		return nil, err
	}

Right, `if` should be a bit different: ``` if err != nil && !(errIsAlreadyExists(err) && w.options.IgnoreAlreadyExistsError) { return nil, err } ```

acid-ant approved these changes 2025-03-05 08:34:28 +00:00

DCO action / DCO (pull_request) Successful in 28s

Required

Details

Code generation / Generate wrappers (pull_request) Successful in 48s

Required

Details

Tests / Tests (pull_request) Successful in 1m0s

Required

Details

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u vd/wait-ctx:vdomnich-yadro-vd/wait-ctx

git checkout vdomnich-yadro-vd/wait-ctx

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project

No assignees

5 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-contract#148

Rows
Columns