-
Notifications
You must be signed in to change notification settings - Fork 7
Add a comments channel, and add exponential backoff and jitter to reconnects #1
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
Changes from all commits
19b9b3b
d9073f0
4faa686
e14fae8
f4d4fc9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,28 +34,31 @@ func NewDecoder(r io.Reader) *Decoder { | |
| // Graceful disconnects (between events) are indicated by an io.EOF error. | ||
| // Any error occuring mid-event is considered non-graceful and will | ||
| // show up as some other error (most likely io.ErrUnexpectedEOF). | ||
| func (dec *Decoder) Decode() (Event, error) { | ||
| // Decoding will return either a successfully parsed Event, a comment, or | ||
| // an error. | ||
| func (dec *Decoder) Decode() (Event, *string, error) { | ||
|
|
||
| // peek ahead before we start a new event so we can return EOFs | ||
| _, err := dec.Peek(1) | ||
| if err == io.ErrUnexpectedEOF { | ||
| err = io.EOF | ||
| } | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err | ||
| } | ||
| pub := new(publication) | ||
| for { | ||
| line, err := dec.ReadString('\n') | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err | ||
| } | ||
| if line == "\n" { | ||
| break | ||
| } | ||
| line = strings.TrimSuffix(line, "\n") | ||
| if strings.HasPrefix(line, ":") { | ||
| continue | ||
| comment := line[1:] | ||
| return nil, &comment, nil | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would probably be good to add a test in |
||
| } | ||
| sections := strings.SplitN(line, ":", 2) | ||
| field, value := sections[0], "" | ||
|
|
@@ -74,5 +77,5 @@ func (dec *Decoder) Decode() (Event, error) { | |
| } | ||
| } | ||
| pub.data = strings.TrimSuffix(pub.data, "\n") | ||
| return pub, nil | ||
| return pub, nil, nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ import ( | |
| "io" | ||
| "io/ioutil" | ||
| "log" | ||
| "math/rand" | ||
| "net/http" | ||
| "time" | ||
| ) | ||
|
|
@@ -25,8 +26,12 @@ type Stream struct { | |
| // action when an error is encountered. The stream will always attempt to continue, | ||
| // even if that involves reconnecting to the server. | ||
| Errors chan error | ||
| // Comments emits any comment lines encountered while reading from the stream | ||
| Comments chan string | ||
| // Logger is a logger that, when set, will be used for logging debug messages | ||
| Logger *log.Logger | ||
| // The maximum time to wait between reconnection attempts | ||
| maxReconnectionTime time.Duration | ||
| } | ||
|
|
||
| type SubscriptionError struct { | ||
|
|
@@ -52,11 +57,13 @@ func Subscribe(url, lastEventId string) (*Stream, error) { | |
| // to be specified, authentication to be configured, etc. | ||
| func SubscribeWithRequest(lastEventId string, req *http.Request) (*Stream, error) { | ||
| stream := &Stream{ | ||
| req: req, | ||
| lastEventId: lastEventId, | ||
| retry: (time.Millisecond * 3000), | ||
| Events: make(chan Event), | ||
| Errors: make(chan error), | ||
| req: req, | ||
| lastEventId: lastEventId, | ||
| retry: (time.Millisecond * 3000), | ||
| Events: make(chan Event), | ||
| Errors: make(chan error), | ||
| Comments: make(chan string), | ||
| maxReconnectionTime: (time.Millisecond * 30000), | ||
| } | ||
| stream.c.CheckRedirect = checkRedirect | ||
|
|
||
|
|
@@ -103,28 +110,66 @@ func (stream *Stream) connect() (r io.ReadCloser, err error) { | |
| return | ||
| } | ||
|
|
||
| func (stream *Stream) backoffWithJitter(attempts int) time.Duration { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make a useful unit test for this method? |
||
| retry := stream.retry.Nanoseconds() | ||
| max := stream.maxReconnectionTime.Nanoseconds() | ||
|
|
||
| exp := pow(2, attempts) | ||
|
|
||
| jitterVal := retry * int64(exp) | ||
|
|
||
| if exp == 0 || jitterVal > max || jitterVal <= 0 { | ||
| jitterVal = max | ||
| } | ||
|
|
||
| return time.Duration(jitterVal/2 + rand.Int63n(jitterVal)/2) | ||
| } | ||
|
|
||
| // Integer power: compute a**b, from Knuth | ||
| func pow(a, b int) int { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. :( I just looked up golang integer exponent, and see that the only built-in exponent function is for float64... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unit test please. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strictly speaking,
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's kind of an infectious change. |
||
| p := 1 | ||
| for b > 0 { | ||
| if b&1 != 0 { | ||
| p *= a | ||
| } | ||
| b >>= 1 | ||
| a *= a | ||
| } | ||
| return p | ||
| } | ||
|
|
||
| func (stream *Stream) stream(r io.ReadCloser) { | ||
| reconnectAttempts := 1 | ||
| defer r.Close() | ||
| dec := NewDecoder(r) | ||
| for { | ||
| ev, err := dec.Decode() | ||
| ev, comment, err := dec.Decode() | ||
|
|
||
| if err != nil { | ||
| stream.Errors <- err | ||
| // respond to all errors by reconnecting and trying again | ||
| break | ||
| } | ||
| pub := ev.(*publication) | ||
| if pub.Retry() > 0 { | ||
| stream.retry = time.Duration(pub.Retry()) * time.Millisecond | ||
|
|
||
| if comment != nil { | ||
| stream.Comments <- *comment | ||
| } | ||
| if len(pub.Id()) > 0 { | ||
| stream.lastEventId = pub.Id() | ||
|
|
||
| if pub, ok := ev.(*publication); ok { | ||
| if pub.Retry() > 0 { | ||
| stream.retry = time.Duration(pub.Retry()) * time.Millisecond | ||
| } | ||
| if len(pub.Id()) > 0 { | ||
| stream.lastEventId = pub.Id() | ||
| } | ||
| stream.Events <- ev | ||
| } else { | ||
| stream.Logger.Printf("Received invalid event") | ||
| } | ||
| stream.Events <- ev | ||
| } | ||
| backoff := stream.retry | ||
| for { | ||
| backoff := stream.backoffWithJitter(reconnectAttempts) | ||
| reconnectAttempts++ | ||
| time.Sleep(backoff) | ||
| if stream.Logger != nil { | ||
| stream.Logger.Printf("Reconnecting in %0.4f secs\n", backoff.Seconds()) | ||
|
|
@@ -139,6 +184,5 @@ func (stream *Stream) stream(r io.ReadCloser) { | |
| break | ||
| } | ||
| stream.Errors <- err | ||
| backoff *= 2 | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| package eventsource | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "testing" | ||
| "time" | ||
| ) | ||
|
|
||
| // This particular "benchmark" exists to spit out various jitter values. It's structured | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tests don't necessarily suppress output. If you use: https://golang.org/pkg/testing/#T.Logf and then run the test with -v you will see the output.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, but I think this still makes more sense as a benchmark than a test, since it doesn't test anything. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either one is fine, just pointing out that output is possible in tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Maybe a better test would be to get a bunch of jittered backoff values, average them, and assert that it is within some threshold of a mean? Or, if the point is to show an example of the values it might spit out, maybe an Example would be more appropriate. |
||
| // as a benchmark so we can see the output, since tests suppress output | ||
| func BenchmarkJitterValues(b *testing.B) { | ||
| str := Stream{ | ||
| retry: 1 * time.Second, | ||
| maxReconnectionTime: 30 * time.Second, | ||
| } | ||
|
|
||
| for i := 0; i < 300; i++ { | ||
| fmt.Printf("Jittered backoff: %v\n", str.backoffWithJitter(i)) | ||
| } | ||
| } | ||
|
|
||
| func TestIntPow(t *testing.T) { | ||
| if pow(2, 4) != 16 { | ||
| t.Errorf("2^4 == 16, got %d", pow(2, 4)) | ||
| } | ||
|
|
||
| if pow(2, 12) != 4096 { | ||
| t.Errorf("2^12 == 4096, got %d", pow(2, 12)) | ||
| } | ||
|
|
||
| if pow(2, 31) != 2147483648 { | ||
| t.Errorf("2^31 == 2147483648, got %d", pow(2, 31)) | ||
| } | ||
|
|
||
| if pow(2, 34) != 17179869184 { | ||
| t.Errorf("2^34 == 17179869184, got %d", pow(2, 34)) | ||
| } | ||
|
|
||
| if pow(2, 300) != 0 { | ||
| t.Errorf("2^300 overflows, expected 0, got %d", pow(2, 300)) | ||
| } | ||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that there are 3 return values, it would be useful in the comments to describe what the *string is that we're returning.