Added token refresh #34

egourlao · 2017-05-22T08:20:20Z

The main addition of this PR is to add a refresh of the account server token before its expiry, which is the suspected cause of #28. It also contains a few refactors and corrections in the network components.
With this fix, token expiry is saved in the network client. A routine is started by the global manager, that updates the token (and the clients created with this token) 2 minutes before a token expires. If the token cannot be refreshed, it is considered as a fatal error and shuts the packet forwarder down.

htdvisser · 2017-05-22T08:42:20Z

pktfwd/manager.go

 func (m *Manager) startRoutines(bgCtx context.Context, err chan error, runTime time.Time) {
-	var errC = make(chan error)
+	var errC = make(chan error, 4)


Why should the channel not block anymore?

In the situation where statusRoutine, networkRoutine and gpsRoutine might all receive an error at the same time, they'll all try to pass it to errC at the same time, which will block these goroutines (since errC is of size 1 currently) and prevent them from receiving the <-bgCtx.Done() information. This means we won't be able to shut these goroutines gracefully. It would probably be better if we let those routines shoot their error, and then - since their cancel function will be called - let them shut gracefully.

You're also making me realize that closing the channel will make the goroutines panic in that situation, so if that's good for you, I'll just remove this close().

Isn't it cleaner for the methods to return their own error channel? This results in multiple error channels, but at least the error owner (i.e. the function) controls the capacity, whether or not to block and close the channel. This is also safer against sending after closing, as the function closes it

johanstokking

The mutexes like this feel a bit sketchy.

Please also consider one goroutine that receives from a channel for actions (functions) to be performed on the connection. That's more asynchronous, but I'm not sure if that's best in this context

johanstokking · 2017-05-22T09:23:30Z

pktfwd/network.go

+	c.routerMutex.Lock()
+	c.connectToStreams(router.NewRouterClientForGateway(router.NewRouterClient(c.currentRouterConn), c.runConfig.ID, c.token), true)
+	c.routerMutex.Unlock()
+	c.uplinkMutex.Unlock()


Unlock in reverse order

johanstokking · 2017-05-22T09:23:57Z

pktfwd/network.go

+	c.connectToStreams(router.NewRouterClientForGateway(router.NewRouterClient(c.currentRouterConn), c.runConfig.ID, c.token), true)
+	c.routerMutex.Unlock()
+	c.uplinkMutex.Unlock()
+	c.downlinkStreamChange <- true


Feels a bit like a callback to me; isn't that cleaner?

It sort of is yes. The idea behind this was that gRPC streams are implemented as channels in Go. When waiting for a downlink, the select block is stuck on a specific channel - downlinkStreamChange signals that the stream needs to be refreshed, and fits well within the select block. What do you think?

johanstokking · 2017-05-22T09:25:27Z

pktfwd/manager.go

 func (m *Manager) startRoutines(bgCtx context.Context, err chan error, runTime time.Time) {
-	var errC = make(chan error)
+	var errC = make(chan error, 4)


Isn't it cleaner for the methods to return their own error channel? This results in multiple error channels, but at least the error owner (i.e. the function) controls the capacity, whether or not to block and close the channel. This is also safer against sending after closing, as the function closes it

johanstokking · 2017-05-22T09:26:52Z

pktfwd/network.go

-		c.uplinkMutex.Lock()
-		c.connectToStreams(router.NewRouterClientForGateway(router.NewRouterClient(routerConn), gatewayID, c.token), true)
-		c.uplinkMutex.Unlock()
+		c.routerMutex.Lock()
 		c.currentRouterConn = routerConn


Afaik all state in a struct in current; no need to prepend a field with that

egourlao · 2017-05-22T12:37:06Z

The latest commit introduces a networkChanges channel in the network client. Whenever a network change needs to be applied (new router, new account server info...), it is passed to this channel as a function that contains the changes to execute. A goroutine then fetches this function, executes it in a safe (locked) environment, and updates subsequently the streams and connections.
The network client also has now less mutexes - only networkMutex, for general network operations (uplink transmission, client changes...), and streamsMutex, for operation regarding streams.
In the manager, there are now separate error channels for every routine (uplinks, statuses, GPS, network).

johanstokking · 2017-05-22T15:46:07Z

pktfwd/manager.go

+	err := m.netClient.RefreshRoutine(bgCtx)
+	if err != nil {
+		errC <- errors.Wrap(err, "Couldn't refresh account server token")
+	}


I meant make an error channel here, and return chan error. This function is owner of that channel and controls open, close, sending and capacity

johanstokking · 2017-05-22T15:47:31Z

pktfwd/network.go

-	return connectionHealthCheck(c.currentRouterConn)
+	c.networkMutex.Lock()
+	t, err := connectionHealthCheck(c.routerConn)
+	c.networkMutex.Unlock()


Defer as much as you can, like here

johanstokking · 2017-05-22T15:48:28Z

pktfwd/network.go

+	close(c.routerChanges)
+	c.streamsMutex.Lock()
+	c.disconnectOfStreams()
+	c.streamsMutex.Unlock()


johanstokking · 2017-05-22T15:50:20Z

pktfwd/network.go

+			if err := routerChange(c); err != nil {
+				c.ctx.WithError(err).Warn("Couldn't operate network client change")
+			}
+			c.connectToStreams(router.NewRouterClientForGateway(router.NewRouterClient(c.routerConn), c.runConfig.ID, c.token))


Should this be executed if routerChange(c) failed?

johanstokking

Check proper closing of channels

Also consider adding one item as capacity to the error channels; there's no reason to block and wait for error handling

johanstokking · 2017-05-24T09:19:02Z

pktfwd/manager.go

+
+			select {
+			case <-bgCtx.Done():
+				errC <- nil


You may close it here?

johanstokking · 2017-05-24T09:19:19Z

pktfwd/manager.go

+		for {
+			select {
+			case <-bgCtx.Done():
+				return


Close the error channel?

johanstokking · 2017-05-24T09:19:36Z

pktfwd/manager.go

+					errC <- errors.Wrap(err, "Gateway status transmission error")
+					return
+				}
+			case <-bgCtx.Done():
 				return


johanstokking

One little channel thing + check all occurrences of closing error channels when you can

johanstokking · 2017-05-29T09:58:01Z

pktfwd/manager.go

+	go func() {
+		if err := m.netClient.RefreshRoutine(bgCtx); err != nil {
+			errC <- errors.Wrap(err, "Couldn't refresh account server token")
+			close(errC)


I like the defer close(errC) approach; if you don't apply it here then the channel stays open

johanstokking · 2017-05-29T10:23:29Z

pktfwd/manager.go

@@ -95,6 +95,8 @@ func (m *Manager) handler(runStart time.Time) (err error) {
 		m.ctx.Error("Program ended after one of the network links failed")
 	}

+	close(c)
+	close(routinesErr)


Use defer. Also, why initialise routinesErr like that?

egourlao · 2017-06-02T11:12:01Z

Is merge OK?

johanstokking

LGTM

Added token refresh

3804e3c

egourlao requested review from htdvisser and johanstokking May 22, 2017 08:20

egourlao added the bug label May 22, 2017

htdvisser reviewed May 22, 2017

View reviewed changes

johanstokking suggested changes May 22, 2017

View reviewed changes

Introduced routerChanges channel for network changes

1566f8f

johanstokking suggested changes May 22, 2017

View reviewed changes

Eric Gourlaouen added 2 commits May 23, 2017 08:40

Routines now returning an error channel

3d01e3d

Merging CRC ignore and new routine format

f63ec91

johanstokking suggested changes May 24, 2017

View reviewed changes

Closing channels at the end of routine

fce2d62

egourlao force-pushed the fix/refresh-token branch from ecd50d6 to fce2d62 Compare May 26, 2017 05:55

johanstokking suggested changes May 29, 2017

View reviewed changes

Adding more & deferred channel closings

d268eef

johanstokking suggested changes May 29, 2017

View reviewed changes

Merged and changed startRoutines specifications

33ac871

johanstokking approved these changes Jun 6, 2017

View reviewed changes

johanstokking merged commit 3f32cf0 into develop Jun 6, 2017

johanstokking deleted the fix/refresh-token branch June 6, 2017 13:10

h0l0gram mentioned this pull request Jun 9, 2017

Packet forwarder stops sending packets after >12 hours of run #28

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added token refresh #34

Added token refresh #34

egourlao commented May 22, 2017

htdvisser May 22, 2017

egourlao May 22, 2017

johanstokking May 22, 2017

johanstokking left a comment

johanstokking May 22, 2017

johanstokking May 22, 2017

egourlao May 22, 2017

johanstokking May 22, 2017

johanstokking May 22, 2017

johanstokking May 22, 2017

egourlao commented May 22, 2017

johanstokking May 22, 2017

johanstokking May 22, 2017

johanstokking May 22, 2017

johanstokking May 22, 2017

johanstokking left a comment

johanstokking May 24, 2017

johanstokking May 24, 2017

johanstokking May 24, 2017

johanstokking left a comment

johanstokking May 29, 2017

johanstokking May 29, 2017

egourlao commented Jun 2, 2017

johanstokking left a comment

Added token refresh #34

Added token refresh #34

Conversation

egourlao commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanstokking 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

egourlao commented May 22, 2017

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

johanstokking 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

johanstokking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egourlao commented Jun 2, 2017

johanstokking left a comment

Choose a reason for hiding this comment