Skip to content
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.

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

remove the txid from the committer list after the listeners have been called #675

Closed
wants to merge 2 commits into from

Conversation

arner
Copy link
Contributor

@arner arner commented Sep 19, 2024

Otherwise the list keeps growing, which causes SQL failures after 1000 transactions and possibly other issues as well.

@adecaro adecaro self-requested a review September 20, 2024 04:34
Copy link
Contributor

@adecaro adecaro left a comment

Choose a reason for hiding this comment

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

@arne, please, can you check the tests?

return
case <-ticker.C:
c.mutex.RLock()
txIDs := c.txIDs.ToSlice()
c.mutex.RUnlock()
if len(txIDs) == 0 {
c.logger.Debugf("no transactions to check vault status")
// c.logger.Debugf("no transactions to check vault status")
break
}

newCtx, span := c.tracer.Start(context.Background(), "vault_status_check")
c.logger.Debugf("check vault status for [%d] transactions", len(txIDs))
Copy link
Contributor

Choose a reason for hiding this comment

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

do you want to remove this?

return
case event := <-c.eventQueue:
c.listenerManager.InvokeListeners(event)
c.mutex.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

this makes me think about handling panics. if InvokeListeners panics, then we don't dispatch events anymore.

@adecaro
Copy link
Contributor

adecaro commented Sep 20, 2024

@arner , please, can you test this commit with token-sdk?

@@ -80,6 +81,7 @@ func (c *FinalityManager[V]) RemoveListener(txID driver.TxID, toRemove driver.Fi
defer c.mutex.Unlock()
c.txIDs.Remove(txID)
c.listenerManager.RemoveListener(txID, toRemove)
// c.logger.Debugf("removed listener for [%s]: now listening to %d transactions...", txID, c.txIDs.Length())
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove them or enable them?

@arner
Copy link
Contributor Author

arner commented Sep 23, 2024

Superseded by #683

@arner arner closed this Sep 23, 2024
@alexandrosfilios alexandrosfilios deleted the f-remove-tx-listeners branch September 23, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants