Merge lp:~allenap/gwacl/poll-immediately into lp:gwacl

[Gavin Panella]

In performPolling, do an immediate poll before sleeping.

[Raphaël Badin (rvb)]

Looks okay, I think it's much harder to follow what's going on now but I don't think it's worth blocking you (although I'm tempted :))

That code is not very readable (especially the new internal poll() method that returns a bool *and* modifies performPolling-level objects)! Part of it, I reckon, is not your fault but is related to the way Go works… but I still wonder if this cannot be improved a tad.

[0]

19 + // Do a single poll, checking for timeout.
20 + poll := func() bool {

Please explain what the returned boolean means.

[1]

20 + poll := func() bool {

Could poll be changed so that it would not modify 'response' and 'err' "in place". That is really confusing, at least to me.

[2]

14 + defer func() {
15 + if err != nil {
16 + response = nil
17 + }
18 + }()

Why did you add that? I suspect it is because an old response from polling number n might be returned along with an error that appended during poll number n+1… please put a comment to explain what purpose this serves.

review: Approve

[lp:~allenap/gwacl/poll-immediately]

155. By Gavin Panella on 2013-07-02

Don't modify variables in enclosing scope.

[Gavin Panella (allenap)]

> [1]
>
> 20      +    poll := func() bool {
>
> Could poll be changed so that it would not modify 'response' and 'err' "in
> place".  That is really confusing, at least to me.

I've updated it to not modify the variables from the enclosing
scope. Please take another look.

[Raphaël Badin (rvb)]

> > [1]
> >
> > 20      +    poll := func() bool {
> >
> > Could poll be changed so that it would not modify 'response' and 'err' "in
> > place".  That is really confusing, at least to me.
>
> I've updated it to not modify the variables from the enclosing
> scope. Please take another look.

Much better I think! More readable, plus you've been able to rid of the response-cleaning deferred.

Thanks for taking the time to do this.

=== modified file 'poller.go'
--- poller.go	2013-04-16 13:09:54 +0000
+++ poller.go	2013-07-02 14:24:22 +0000
@@ -22,22 +22,37 @@
 func performPolling(poller poller, interval time.Duration, timeout time.Duration) (*x509Response, error) {
     timeoutChannel := time.After(timeout)
     ticker := time.Tick(interval)
-    for _ = range ticker {
+    // Function to do a single poll, checking for timeout. The bool returned
+    // indicates if polling is finished, one way or another.
+    poll := func() (bool, *x509Response, error) {
         // This may need to tolerate some transient failures, such as network
         // failures that may go away after a few retries.
         select {
         case <-timeoutChannel:
-            return nil, fmt.Errorf("polling timed out waiting for an asynchronous operation")
+            return true, nil, fmt.Errorf("polling timed out waiting for an asynchronous operation")
         default:
             response, pollerErr := poller.poll()
             done, err := poller.isDone(response, pollerErr)
             if err != nil {
-                return nil, err
+                return true, nil, err
             }
             if done {
-                return response, nil
+                return true, response, nil
             }
         }
+        return false, nil, nil
+    }
+    // Do an initial poll.
+    done, response, err := poll()
+    if done {
+        return response, err
+    }
+    // Poll every interval.
+    for _ = range ticker {
+        done, response, err := poll()
+        if done {
+            return response, err
+        }
     }
     // This code cannot be reached but Go insists on having a return or a panic
     // statement at the end of this method.  Sigh.

=== modified file 'poller_test.go'
--- poller_test.go	2013-06-27 10:55:35 +0000
+++ poller_test.go	2013-07-02 14:24:22 +0000
@@ -62,6 +62,16 @@
     return false, nil
 }
 
+func (suite *pollerSuite) TestperformPollingPollsOnceImmediately(c *C) {
+    poller := newTestPoller(0, 0)
+    interval := time.Second * 10
+    start := time.Now()
+    response, err := performPolling(poller, interval, interval*2)
+    c.Assert(err, Equals, nil)
+    c.Assert(time.Since(start) < interval, Equals, true)
+    c.Assert(response, DeepEquals, &testPollerResponse)
+}
+
 func (suite *pollerSuite) TestperformPollingReturnsError(c *C) {
     poller := newTestPoller(0, 1)
     response, err := performPolling(poller, time.Nanosecond, time.Minute)