horizonclient: review setCurrentServerTime

[poliha]

The current implementation has some issues when used in goroutines see #1276 and #1284
This issue is to track review and possible improvements

[ire-and-curses]

I helped a bit with the debugging and I think there are a couple of things to look at.

1. Fix shared writable state. Go maps are not thread safe. More generally, anything that is top-level shared writable state is not going to be safe. The PR's mentioned above work around the access problem for this map. A question to ask is "is there a simpler way to store this state"? If it's not global (i.e. never accessed by more than one goroutine) then it won't be a problem and we could remove the mutexes.

If it's not possible to make this local, then one option is to switch to a concurrent map implementation. Otherwise, any future use of the map could potentially break concurrency if the author forgets to mutex.

We should confirm that there is no other shared state at the top-level, other than read-only variables.

2. Failing unit test.

@debnil found this failure during CI:

--- FAIL: TestFetchTimebounds (0.15s)
panic: runtime error: index out of range [recovered]
    panic: runtime error: index out of range

goroutine 80 [running]:
testing.tRunner.func1(0xc4202ddd10)
    /usr/local/go/src/testing/testing.go:742 +0x29d
panic(0x8b7140, 0xc334d0)
    /usr/local/go/src/runtime/panic.go:502 +0x229
github.com/stellar/go/clients/horizonclient.setCurrentServerTime(0xc4202fade8, 0x13, 0x0, 0x0, 0x0)
    /go/src/github.com/stellar/go/clients/horizonclient/internal.go:114 +0x324
github.com/stellar/go/clients/horizonclient.decodeResponse(0xc4202de000, 0x870500, 0xc42038d920, 0x0, 0x0)
    /go/src/github.com/stellar/go/clients/horizonclient/internal.go:20 +0x303
github.com/stellar/go/clients/horizonclient.(*Client).sendRequestURL(0xc3ac00, 0xc4202fade0, 0x23, 0x94325c, 0x3, 0x86ed80, 0xc420335000, 0xc4202c8af8, 0xc42038d910)
    /go/src/github.com/stellar/go/clients/horizonclient/client.go:67 +0x25f
github.com/stellar/go/clients/horizonclient.(*Client).sendRequest(0xc3ac00, 0x9bb8a0, 0xc42038d910, 0x86ed80, 0xc420335000, 0xc4202c8ca0, 0x94b9ad)
    /go/src/github.com/stellar/go/clients/horizonclient/client.go:37 +0x1c7
github.com/stellar/go/clients/horizonclient.(*Client).Metrics(0xc3ac00, 0x411e79, 0x41516c, 0x4120c8, 0xc4202bfae0, 0x10, 0x49a00c, 0x49a054, 0x190, 0xa, ...)
    /go/src/github.com/stellar/go/clients/horizonclient/client.go:301 +0xa2
github.com/stellar/go/clients/horizonclient.TestFetchTimebounds(0xc4202ddd10)
    /go/src/github.com/stellar/go/clients/horizonclient/main_test.go:1024 +0x4a
testing.tRunner(0xc4202ddd10, 0x979830)
    /usr/local/go/src/testing/testing.go:777 +0xd0
created by testing.(*T).Run

This test probably failed because of a rare pubnet failure during the time of the outage. Issues to resolve:

1.1 There is a corner case where somehow we receive a response, but it contains no date. This means the error check wrapping the call to setCurrentServerTime is not sufficient to guarantee the date will be present.
1.2 If we don't have a date, what should we do? Do we have a sensible default? We could consider raising an error if this is important.
1.3 If the theory is correct, how was friendbot, running on testnet, affected by a failure of pubnet? That shouldn't be possible. Are we pointing at the wrong Horizon, or somehow bleeding state between clients? Let's check the code again.
1.4 The test TestFetchTimebounds is not good. It is hitting the network with client.Metrics() to set up a server time as a side-effect. Both of these things are problematic. We should rewrite this test to use a mock.

[bartekn]

I checked the code around setCurrentServerTime, currentServerTime. One thing that caught my eye is that reads from the map in currentServerTime are not protected with mutex. According to Go maps in action to use the map concurrently RWMutex should be used.

[ire-and-curses]

@bartek yeah agree. We have https://github.com/stellar/go/pull/1284/files to address that but not merged yet I think

[poliha]

Before we figure out if we can make the map local, we should unexport it and add a note about using mutex with the map.
About the test against pubnet, I opted for this at the time because we currently don't mock request headers and making the change was out of the scope of what I was working on. Mocking against pubnet which always has a live horizon server was the closest thing to usage in the wild.
I have recently reviewed support/http/httptest and I have identified a way we can support mocking request headers. Will make a PR soon.

Yes there is corner case where there might not be a Date in the response headers. The default action here is to do nothing. When this happens, Client.FetchTimebounds will default to using local time.
I guess the only way to gaurantee that Date will be part of the response will be for horizon to return it. If not, there is always the possibility of proxy webservers overwriting or not returning this in the headers.

Also your theory about the failing CI due to pubnet failure is correct because the test uses the DefaultPubnetClient

[ire-and-curses]

Related: #1322, #1323