Result of tx processing returned as QueuedTxResult #537

dshulyak · 2018-01-06T09:01:50Z

Currently, it is quite easy to introduce concurrency issues while working
with transaction object. For example, race issue will exist every time
while a transaction is processed in a separate goroutine and caller will
try to check for an error before an event to the Done channel is sent.

Current change removes all the data that is updated on transaction and leaves
it with ID, Args, and Context (which is not used at the moment).

Blocked by: #530

divan · 2018-01-09T19:02:17Z

Thanks for PR, it's really nice idea to split QueueTx into two types. One thing I can't figure out at the moment — what's the purpose of having Tx QueuedTx field in the QueuedTxResult type?

Looks like every call of WaitForTransaction(tx, c) has an access to the original QueuedTx variable anyway. And I don't see any real usage of that Tx field.

Also, I struggle with a newWaitForTransaction() signature: it accepts QueuedTx and channel of QueuedTxResults as parameters, but those two things should be logically connected. Package API should not allow calling WaitForTransaction() with Tx1 and channel of results for Tx2, for example, right?. I cannot quickly see how to redesign it though.

With current status-react-status-go API it looks like we use QueueTransaction() and WaitForTransaction() together, so maybe it makes sense to simply merge QueueTransaction() and WaitTransaction()?

cc @adambabik

dshulyak · 2018-01-09T20:04:49Z

@divan no reason, I was pretty sure that it was removed. When I added it I wanted to get rid from tx passed to WaitTransaction, but then I noticed that I can't.

I can add Result field to a tx, and then it won't be possible to mix tx1 with tx2 result channel.
For some reason i dislike such channel on objects, but it still will be safe to work with transactions

type QueuedTx struct {
	ID      QueuedTxID
	Context context.Context
	Args    SendTxArgs
	Result chan QueuedTxResult 
}

divan · 2018-01-09T20:19:12Z

@dshulyak that makes sense now. Yeah, I guess the issue here is that the channel used only once for getting a single result, and it feels weird because we used to use channels for a stream of data and not just single value. But I think that's fine.

divan · 2018-01-18T16:58:07Z

@dshulyak can you please check conflicts with develop?

dshulyak · 2018-01-22T10:14:35Z

rebased, ready for review

themue

As we're inside of package transactions I'm asking myself if it's a good opportunity to drop the prefix Tx from TxQueue and TxQueueManager?

themue · 2018-01-26T12:59:41Z

geth/api/backend.go

 	tx := common.CreateTransaction(ctx, args)
-
 	if err := m.txQueueManager.QueueTransaction(tx); err != nil {


err := now can be err = .

dshulyak · 2018-01-26T14:08:39Z

As we're inside of package transactions I'm asking myself if it's a good opportunity to drop the prefix Tx from TxQueue and TxQueueManager?

this is good idea. i am planning to create a pr where i will remove QueuedTxID. I will also include mentioned cleanup

mandrigin

Just a few notes, looks good otherwise.

mandrigin · 2018-01-29T18:41:27Z

geth/common/types.go

@@ -206,7 +204,7 @@ type TxQueueManager interface {
 	QueueTransaction(tx *QueuedTx) error

 	// WaitForTransactions blocks until transaction is completed, discarded or timed out.
-	WaitForTransaction(tx *QueuedTx) error
+	WaitForTransaction(tx *QueuedTx) RawCompleteTransactionResult


Doesn't Result part assume that transaction is completed? I'd renamed it to RawTransactionResult if that makes sense.

I would even change it to TransactionResult.

mandrigin · 2018-01-29T18:42:13Z

geth/transactions/notifications.go

-	// discard notifications with empty tx
-	if queuedTx == nil {
+func NotifyOnReturn(queuedTx *common.QueuedTx, err error) {
+	// we don't want to notify a user if tx wassent successfully


typo: was sent

mandrigin · 2018-01-29T18:45:47Z

geth/transactions/txqueue_manager.go

+		txQueue:               queue.New(),
+		addrLock:              &AddrLocker{},
+		notify:                true,
+		sendCompletionTimeout: DefaultTxSendCompletionTimeout * time.Second,


I would move * time.Second to the const declaration instead.

mandrigin · 2018-01-29T18:49:14Z

geth/transactions/txqueue_manager.go

+		select {
+		case rst := <-tx.Result:
+			return rst
+		case <-time.After(m.sendCompletionTimeout):


Does it make sense to rename it to completionTimeout?

mandrigin · 2018-01-29T18:50:10Z

geth/transactions/txqueue_manager.go

 	}
-
-	return tx.Hash.Hex(), nil
+	// handle empty hash


Is it a TODO?

i think i forgot to remove, hash cant be empty if err is nil now

adambabik · 2018-01-30T07:51:56Z

geth/transactions/notifications.go

-			ErrorMessage: queuedTx.Err.Error(),
-			ErrorCode:    strconv.Itoa(sendTransactionErrorCode(queuedTx.Err)),
+			ErrorMessage: err.Error(),
+			ErrorCode:    strconv.Itoa(sendTransactionErrorCode(err)),


Why can't ErrorCode be an integer?

because it is expected on consumer side? i can make this conversion when marshaling though

But on the consumer side, it can be an integer and it should be ok. Here, it will just convert 1 to "1". I don't think it will make any difference to status-react.

adambabik · 2018-01-30T07:56:00Z

geth/common/types.go

@@ -206,7 +204,7 @@ type TxQueueManager interface {
 	QueueTransaction(tx *QueuedTx) error

 	// WaitForTransactions blocks until transaction is completed, discarded or timed out.
-	WaitForTransaction(tx *QueuedTx) error
+	WaitForTransaction(tx *QueuedTx) RawCompleteTransactionResult


I would even change it to TransactionResult.

adambabik · 2018-01-30T08:04:18Z

geth/transactions/txqueue_manager.go

 	if m.notify {
 		NotifyOnEnqueue(tx)
 	}
-	return err
+	return nil
 }

 func (m *Manager) txDone(tx *common.QueuedTx, hash gethcommon.Hash, err error) {
 	m.txQueue.Done(tx.ID, hash, err) //nolint: errcheck


I think we should handle this error because why not?

missed this at first, the only error that can be reported is that tx is not found
i think there is only 3-4 cases when this error can be reported:

tx discarded but then completed

tx timed out but then completed, or vice versa

and maybe

discarded and then timed out

i think in all of them it is safe just to exit without notification, will add tomorrow

I would at least log a warning message as it's pretty unusual situation. It will help to debug if there will be any issues with the app.

adambabik · 2018-01-30T08:11:59Z

geth/common/utils.go

 		Context: ctx,
 		Args:    args,
-		Done:    make(chan struct{}),
+		Result:  make(chan RawCompleteTransactionResult, 1),


Generally, I am not a fan of using a buffered channel to pass the result because it can be read only once. I assume it's used instead of mutex and getters/setters. On the other hand, if it works fine now and is not something that limits us then let's leave it.

adambabik · 2018-01-30T08:18:10Z

And one more thing @dshulyak, let's verify that transactions/... unit tests do not report any race conditions when running with -race flag. It was a problem before.

dshulyak · 2018-01-30T08:54:51Z

thanks for review, i will address comments today

@adambabik as for race conditions, i fixed them in the previous PR, so this change just adds additional guarantees that they won't be added again

go test -race ./geth/transactions/...
ok github.com/status-im/status-go/geth/transactions 1.938s
? github.com/status-im/status-go/geth/transactions/fake [no test files]
ok github.com/status-im/status-go/geth/transactions/queue 1.964s

dshulyak · 2018-01-31T07:20:31Z

@adambabik @mandrigin @themue can we get this in? i will address the last thing in next PR #538

adambabik

We are pretty close!

adambabik · 2018-01-31T06:59:10Z

geth/transactions/notifications.go

@@ -61,17 +59,17 @@ type ReturnSendTransactionEvent struct {
 	Args         common.SendTxArgs `json:"args"`
 	MessageID    string            `json:"message_id"`
 	ErrorMessage string            `json:"error_message"`
-	ErrorCode    string            `json:"error_code"`
+	ErrorCode    int               `json:"error_code,string"`


Ah, I see now that ErrorCode was defined as a string in a struct. This is a nice solution!

adambabik · 2018-01-31T07:03:47Z

geth/transactions/queue/queue.go

@@ -133,9 +133,6 @@ func (q *TxQueue) Reset() {
 // Enqueue enqueues incoming transaction
 func (q *TxQueue) Enqueue(tx *common.QueuedTx) error {
 	log.Info(fmt.Sprintf("enqueue transaction: %s", tx.ID))
-	if (tx.Hash != gethcommon.Hash{} || tx.Err != nil) {


Ok, I remember when it can happen that a transaction is queued multiple times. It's possible from the interface. We should check if the given transaction is not in the q.transactions already.

you meant transaction managers interface, right? not one that is used by react, i don't see how react can set ID for queued transaction

adambabik · 2018-01-31T07:07:25Z

geth/transactions/txqueue_manager.go

+		case rst := <-tx.Result:
+			return rst
+		case <-time.After(m.completionTimeout):
+			m.txDone(tx, gethcommon.Hash{}, queue.ErrQueuedTxTimedOut)


Not sure if ErrQueuedTxTimedOut should be in queue package. queue package does not know anything about timing out.

adambabik · 2018-01-31T07:08:09Z

geth/transactions/txqueue_manager.go

@@ -244,7 +250,7 @@ func (m *Manager) DiscardTransaction(id common.QueuedTxID) error {
 	}
 	err = m.txQueue.Done(id, gethcommon.Hash{}, queue.ErrQueuedTxDiscarded)
 	if m.notify {
-		NotifyOnReturn(tx)
+		NotifyOnReturn(tx, queue.ErrQueuedTxDiscarded)


Similarly, maybe ErrQueuedTxDiscarded should be defined in Manager? queue also has no notion of discarding.

adambabik · 2018-01-31T07:16:47Z

geth/transactions/txqueue_manager_test.go

@@ -197,10 +197,9 @@ func (s *TxQueueTestSuite) TestAccountMismatch() {
 		To:   common.ToAddress(TestConfig.Account2.Address),
 	})

-	err := txQueueManager.QueueTransaction(tx)
-	s.NoError(err)
+	txQueueManager.QueueTransaction(tx)


In some places, the error is checked

s.NoError(txQueueManager.QueueTransaction(tx))

and in here it's not. It should be consistent. Btw. why does linter not complain that error is not checked?

maybe linter doesn't complain in tests

You're right. We would need to add "Test": true to .gometalinter.json to have test files checked.

adambabik · 2018-01-31T07:23:42Z

geth/transactions/txqueue_manager.go

 	if m.notify {
 		NotifyOnEnqueue(tx)
 	}
-	return err
+	return nil
 }

 func (m *Manager) txDone(tx *common.QueuedTx, hash gethcommon.Hash, err error) {
 	m.txQueue.Done(tx.ID, hash, err) //nolint: errcheck


I would at least log a warning message as it's pretty unusual situation. It will help to debug if there will be any issues with the app.

adambabik

Everything looks good except for one place where a deadlock is possible.

adambabik · 2018-01-31T11:41:16Z

geth/transactions/queue/queue.go

-		return ErrQueuedTxAlreadyProcessed
+	q.mu.RLock()
+	if _, ok := q.transactions[tx.ID]; ok {
+		return ErrQueuedTxExist


Deadlock possibility as q.mu.RUnlock() may never be executed. It also suggests there is no unit test for this case.

adambabik · 2018-01-31T11:42:32Z

geth/transactions/txqueue_manager.go

@@ -75,34 +77,44 @@ func (m *Manager) QueueTransaction(tx *common.QueuedTx) error {
 		to = tx.Args.To.Hex()
 	}
 	log.Info("queue a new transaction", "id", tx.ID, "from", tx.Args.From.Hex(), "to", to)
-	err := m.txQueue.Enqueue(tx)
+	if err := m.txQueue.Enqueue(tx); err == queue.ErrQueuedTxExist {
+		log.Warn("Transaction was already enqueud", tx.ID)


Typo: s/enqueud/enqueued

Any particular reason why we don't want to notify about this error? It may be beneficial for status-react as this also indicates a bug in the UI. UI should prevent from submitting the same transaction twice.

i thought that warning will be enough to catch this

ok, lets leave things how they were but with warning

I'm opinionated, but warning is a least useful log level :) Why not print it as an error, if we are sure that txqueue client (status-react I mean) should guarantee there is no double enqueueing?

actually status react cant cause this error, it just sends from, to and amount. and we generate a random uid for that

i would prefer just to ignore this error, as it doesn't lead to any real problem. but i will just pass it up the stack

Whatever makes more sense. The logic is that the decision "this condition is fine/is erroneous" should be codified. Warning level is literally asking a human to decide if it's fine or not. And humans often don't know the answer :)

Currently it is quite easy to introduce concurrency issues while working with transaction object. For example, race issue will exist every time while transaction is processed in a separate goroutine and caller will try to check for an error before event to Done channel is sent. This change removes all the data that is updated on transaction and leaves it with ID, Args and Context (which is not used at the moment). Signed-off-by: Dmitry Shulyak <[email protected]>

dshulyak · 2018-01-31T15:39:54Z

@adambabik @mandrigin folks, any reason not to merge this change?

adambabik

LGTM

mandrigin

LGTM

dshulyak force-pushed the tximutable branch 3 times, most recently from 2c91b13 to d75f6f0 Compare January 6, 2018 11:20

dshulyak mentioned this pull request Jan 8, 2018

Refactoring of TxQueue and Manager #530

Merged

dshulyak force-pushed the tximutable branch from d75f6f0 to aa15a9f Compare January 10, 2018 07:39

dshulyak force-pushed the tximutable branch from aa15a9f to bcf7d47 Compare January 22, 2018 10:13

dshulyak force-pushed the tximutable branch 2 times, most recently from 06d0ec1 to 81647fb Compare January 26, 2018 11:26

dshulyak mentioned this pull request Jan 26, 2018

Maintain local copy of the nonce for each used address #538

Merged

themue reviewed Jan 26, 2018

View reviewed changes

mandrigin suggested changes Jan 29, 2018

View reviewed changes

adambabik reviewed Jan 30, 2018

View reviewed changes

dshulyak force-pushed the tximutable branch 2 times, most recently from 11cbd95 to afa68ca Compare January 30, 2018 14:49

adambabik suggested changes Jan 31, 2018

View reviewed changes

dshulyak force-pushed the tximutable branch 4 times, most recently from f086496 to 2dae2eb Compare January 31, 2018 10:59

adambabik suggested changes Jan 31, 2018

View reviewed changes

dshulyak force-pushed the tximutable branch 3 times, most recently from c139d5d to 2ff58d1 Compare January 31, 2018 12:59

dshulyak force-pushed the tximutable branch from 2ff58d1 to d325ef7 Compare January 31, 2018 13:15

adambabik approved these changes Jan 31, 2018

View reviewed changes

mandrigin approved these changes Jan 31, 2018

View reviewed changes

dshulyak merged commit 653da5b into status-im:develop Feb 2, 2018

dshulyak mentioned this pull request Feb 2, 2018

Cover transaction manager with additional test cases #561 #601

Merged

canercidam added a commit to canercidam/status-go that referenced this pull request Feb 2, 2018

merge fixes from status-im#537

e3d708a

		tx := common.CreateTransaction(ctx, args)

		if err := m.txQueueManager.QueueTransaction(tx); err != nil {

Result of tx processing returned as QueuedTxResult #537

Result of tx processing returned as QueuedTxResult #537

Conversation

dshulyak commented Jan 6, 2018 • edited Loading

divan commented Jan 9, 2018

dshulyak commented Jan 9, 2018

divan commented Jan 9, 2018

divan commented Jan 18, 2018

dshulyak commented Jan 22, 2018

themue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dshulyak commented Jan 26, 2018

mandrigin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adambabik commented Jan 30, 2018

dshulyak commented Jan 30, 2018

dshulyak commented Jan 31, 2018

adambabik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adambabik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divan Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divan Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

dshulyak commented Jan 31, 2018 • edited Loading

adambabik left a comment

Choose a reason for hiding this comment

mandrigin left a comment

Choose a reason for hiding this comment

dshulyak commented Jan 6, 2018 •

edited

Loading

divan Jan 31, 2018 •

edited

Loading

divan Jan 31, 2018 •

edited

Loading

dshulyak commented Jan 31, 2018 •

edited

Loading