Ticker Package #1668

cfromknecht · 2018-08-01T22:24:47Z

This PR is an extension of #1659, and solidifies a generic ticker.Ticker interface for resumable tickers used within the switch. It replaces the existing tickers within the link and switch with the new resumable ticker, even though only the link's batch ticker actually utilizes the conditional aspect. However, this will allow for similar optimizations to made if others are found to be hotspots.

This is required for #1551, as #1659 broke the previous method for testing link shutdowns. Previously, we used the batch ticker to test when the link becomes blocked during forwarding, so that we can test the ability for the link to be Stop() while synchronously forwarding Adds to the switch. The primary reason for this PR is thus to add the ticker.Mock, which satisfies the behavioral requirements of the production ticker, but also allows us to force feed ticks externally and arbitrarily.

halseth

Niiiiice, having a generic ticker interface should make testing much easier

halseth · 2018-08-02T10:04:56Z

ticker/ticker.go

+// NOTE: Part of the Ticker interface.
+func (t *ticker) Resume() {
+	t.ticker = time.NewTicker(t.interval)
+	t.ticks = t.ticker.C


seems like it would be simpler to just store t.ticker, and return t.ticker.C from Ticks()

I believe ticker.C is buffered, so calling Stop() might still produce an extra tick. See this example: https://play.golang.org/p/5I4Cm35wyao

Swapping out the channel for nil ensures that this won't produce a tick when selected on

I mean, in Tick we can do

if t.ticker == nil { return nil } return t.ticker.C

Should be equivalent?

halseth · 2018-08-02T10:05:59Z

htlcswitch/link.go

@@ -1028,8 +986,8 @@ out:
 			// If the downstream packet resulted in a non-empty
 			// batch, reinstate the batch ticker so that it can be
 			// cleared.
-			if l.batchCounter > 0 && maybeBatchTick == nil {
-				maybeBatchTick = l.cfg.BatchTicker.Start()
+			if l.batchCounter > 0 && !l.cfg.BatchTicker.IsActive() {


shouldn't need to check IsActive here?

I don't think we want to resume if we're already active, could cause us to reinitialize a ticker every time the batch is non-empty

We could make Resume a no-op if the ticker is already active.

oh great idea!

halseth · 2018-08-02T10:10:20Z

ticker/ticker.go

+	// false.
+	//
+	// NOTE: The behavior of a Ticker is undefined after calling Stop.
+	Stop()


Do we need to distinguish pausing from stopping at this point? I think we should stick to Start/Stop as that should be enough for our use case.

Start/Stop is enough, but the mock ticker has a goroutine that needs to be cleaned up. In order to handle forced ticks while paused, i.e. keeping the goroutine alive, there needs to be a distinction between pausing, and actually tearing down the ticker

Still don't think a mock implementation detail should clutter up the interface, but I surrender to the overlords 🙇

wpaulino

Nice! I dig the new approach of the mock ticker where we can force ticks to trigger specific behaviors 😎

wpaulino · 2018-08-03T00:27:23Z

ticker/ticker.go

+
+// New returns a new ticker that signals with the given interval
+// when not paused.
+func New(interval time.Duration) *ticker {


This should return the interface instead, otherwise we'd be returning an unexported identifier from an exported one.

Discussed in person, leaving as concrete type

wpaulino · 2018-08-03T00:29:08Z

ticker/mock.go

+	"time"
+)
+
+// Mock implements the htlcswitch.Ticker interface, and provides a method of


s/htlcswitch.Ticker/ticker.Ticker/

wpaulino · 2018-08-03T00:32:11Z

ticker/mock.go

+}
+
+// NOTE: Part of the Ticker interface.
+func (m *Mock) Ticks() <-chan time.Time {


Missing godoc, as well as all the others below.

wpaulino · 2018-08-03T00:50:29Z

peer.go

@@ -416,6 +417,7 @@ func (p *peer) loadActiveChannels(chans []*channeldb.OpenChannel) error {
 		)
 		if err != nil {
 			lnChan.Stop()
+			peerLog.Debugf("unable to add link %v: %v", chanPoint, err)


Nit: capitalize first word for consistency.

fixed! also made peer disconnection logged to info, which seems useful as per discussion in #1630

Roasbeef

LGTM! 🏒

The code reads well to me, and will definitely allow us to effectively test any scenario which depends on ticker behavior within the codebase. Before I merge this, will run it on the faucet for a bit to ensure that there hasn't been a regression in the improvements for idle CPU time.

halseth

LGTM ⏰

halseth · 2018-08-09T09:12:08Z

ticker/ticker.go

+	Pause()
+
+	// Stop suspends the underlying ticker, such that Ticks() stops
+	// signaling at regular intervals,  and permanently frees up any


nit: extra white space

halseth · 2018-08-09T09:13:51Z

ticker/ticker.go

+	// false.
+	//
+	// NOTE: The behavior of a Ticker is undefined after calling Stop.
+	Stop()


Still don't think a mock implementation detail should clutter up the interface, but I surrender to the overlords 🙇

halseth · 2018-08-09T09:14:20Z

ticker/ticker.go

+}
+
+// ticker is the production implementation of the resumable Ticker interface.
+// This allows various components of the htlcswitch to toggle their need for


nit: remove htlcswitch reference

halseth · 2018-08-09T09:19:14Z

htlcswitch/link.go

-			if l.batchCounter > 0 && maybeBatchTick == nil {
-				maybeBatchTick = l.cfg.BatchTicker.Start()
+			if l.batchCounter > 0 {
+				l.cfg.BatchTicker.Resume()


so beautiful 😻

halseth · 2018-08-09T09:22:06Z

peer.go

@@ -576,7 +578,7 @@ func (p *peer) Disconnect(reason error) {
 		return
 	}

-	peerLog.Tracef("Disconnecting %s, reason: %v", p, reason)
+	peerLog.Infof("Disconnecting %s, reason: %v", p, reason)


cfromknecht · 2018-08-09T09:47:13Z

comments addressed!

re: interface clutter, it's not necessarily the mock ticker that's dictating this. In my mind, the aim is to generically encompass the possibility of tickers requiring cleanup that is different from Pause (same for start/resume), of which the mock implementation is one example. Not differentiating assumes that there is no useful ticker requiring these distinctions, even though the mock implementation is what led to me to think that there might be such a case :)

This commit adds a Mock Ticker, implementing the Ticker interface. The ticker abides by the behavioral requirements of the production implementation, but also includes the ability to force feed ticks while the Ticker is paused. This commit also places the mock.go file behind the debug build flag. This allows us to import the MockTicker into our unit tests.

resumable ticker.Ticker interface

cfromknecht self-assigned this Aug 1, 2018

cfromknecht added htlcswitch P2 should be fixed if one has time needs review PR needs review by regular contributors labels Aug 1, 2018

cfromknecht force-pushed the interface-tickers branch from 3b9d6e0 to f12d2fe Compare August 1, 2018 22:38

cfromknecht mentioned this pull request Aug 1, 2018

[htlcswitch]: revert replace link, ensure removed links are stopped #1551

Merged

cfromknecht force-pushed the interface-tickers branch 7 times, most recently from 30decf4 to eb5e221 Compare August 2, 2018 09:37

cfromknecht changed the title ~~htlcswitch: Interface tickers~~ Ticker Package Aug 2, 2018

cfromknecht force-pushed the interface-tickers branch from eb5e221 to 6b90032 Compare August 2, 2018 09:50

cfromknecht added the testing Improvements/modifications to the test suite label Aug 2, 2018

halseth reviewed Aug 2, 2018

View reviewed changes

cfromknecht force-pushed the interface-tickers branch 2 times, most recently from 2e5a269 to bf5f7a3 Compare August 2, 2018 12:07

wpaulino reviewed Aug 3, 2018

View reviewed changes

cfromknecht force-pushed the interface-tickers branch 2 times, most recently from 9d50fea to e1acd1a Compare August 3, 2018 02:31

cfromknecht mentioned this pull request Aug 3, 2018

[server]: Start Peers Asynchronously #1658

Merged

2 tasks

cfromknecht force-pushed the interface-tickers branch 3 times, most recently from 5494114 to 9381dee Compare August 4, 2018 02:16

Roasbeef added this to the 0.5 milestone Aug 8, 2018

Roasbeef added needs testing PR hasn't yet been actively tested on testnet/mainnet and removed needs review PR needs review by regular contributors labels Aug 9, 2018

Roasbeef previously approved these changes Aug 9, 2018

View reviewed changes

cfromknecht dismissed Roasbeef’s stale review via 139148d August 9, 2018 03:40

cfromknecht force-pushed the interface-tickers branch 3 times, most recently from bb43c81 to c532718 Compare August 9, 2018 07:41

halseth previously approved these changes Aug 9, 2018

View reviewed changes

cfromknecht dismissed halseth’s stale review via 63149ed August 9, 2018 09:33

cfromknecht force-pushed the interface-tickers branch from c532718 to 63149ed Compare August 9, 2018 09:33

cfromknecht added 11 commits August 9, 2018 02:48

ticker/ticker: adds resumable Ticker interface

f8fe26f

ticker/ticker_test: adds Ticker interface tests

83353b4

htlcswitch/link: replace batch ticker with...

1c456a5

resumable ticker.Ticker interface

htlcswitch/switch: add log/fwd event tickers to Config

e3253a4

htlcswitch/link_test: replace mockTicker with ticker.Mock

2651136

htlcswitch/mock: remove mockTicker, use ticker.MockNew

6a70952

htlcswitch/test_utils: repalce mockTicker with new MockTicker

c90ec19

peer: replace NewBatchTicker with ticker.New

2762546

server: configure switch w/ default log/fwd tickers

d5d01dd

test_utils: init switch with default fwd/log tickers

8cc4681

cfromknecht force-pushed the interface-tickers branch from 63149ed to 8cc4681 Compare August 9, 2018 09:48

halseth approved these changes Aug 9, 2018

View reviewed changes

Roasbeef mentioned this pull request Aug 9, 2018

travis - allow tickers to tick for coverage #1641

Closed

Roasbeef merged commit d3b1b9a into lightningnetwork:master Aug 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ticker Package #1668

Ticker Package #1668

cfromknecht commented Aug 1, 2018 •

edited

Loading

halseth left a comment

halseth Aug 2, 2018

cfromknecht Aug 2, 2018

halseth Aug 2, 2018

cfromknecht Aug 2, 2018

halseth Aug 2, 2018

cfromknecht Aug 2, 2018

halseth Aug 2, 2018

cfromknecht Aug 2, 2018

halseth Aug 2, 2018

cfromknecht Aug 2, 2018 •

edited

Loading

halseth Aug 9, 2018

wpaulino left a comment

wpaulino Aug 3, 2018

cfromknecht Aug 3, 2018

wpaulino Aug 3, 2018

cfromknecht Aug 3, 2018

wpaulino Aug 3, 2018

cfromknecht Aug 3, 2018

wpaulino Aug 3, 2018

cfromknecht Aug 3, 2018

Roasbeef left a comment •

edited

Loading

halseth left a comment

halseth Aug 9, 2018

halseth Aug 9, 2018

halseth Aug 9, 2018

cfromknecht Aug 9, 2018

halseth Aug 9, 2018

halseth Aug 9, 2018

cfromknecht Aug 9, 2018

cfromknecht commented Aug 9, 2018

Ticker Package #1668

Ticker Package #1668

Conversation

cfromknecht commented Aug 1, 2018 • edited Loading

halseth 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

cfromknecht Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpaulino 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

Roasbeef left a comment • edited Loading

Choose a reason for hiding this comment

halseth 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

cfromknecht commented Aug 9, 2018

cfromknecht commented Aug 1, 2018 •

edited

Loading

cfromknecht Aug 2, 2018 •

edited

Loading

Roasbeef left a comment •

edited

Loading