Fix/networkidle review

[inancgumus]

This one uses Go 1.19's atomic.Int64, so test-prev (which is Go 1.18) fails... WDYT?

[ankur22]

This one uses Go 1.19's atomic.Int64, so test-prev (which is Go 1.18) fails... WDYT?

What's the benefit of doing this change over what we already have?

I think we should work with what is backward compatible for now, and create a ticket to update to Go 1.19 specific things only if it enhances or solves a problem that we're facing.

[inancgumus]

This one uses Go 1.19's atomic.Int64, so test-prev (which is Go 1.18) fails... WDYT?

What's the benefit of doing this change over what we already have?

I'm not sure I understand your question. Are we discussing atomic vs. mutexes? If that's the case, an atomic counter (which you probably know already) is simpler, easier, and faster to concurrently count numeric data than a mutex.

For, Go 1.19 stuff, I agree, so we can just use atomic.AddInt64 instead (Go 1.18 compatible).

[ankur22]

I'm not sure I understand your question. Are we discussing atomic vs. mutexes? If that's the case, an atomic counter (which you probably know already) is simpler, easier, and faster to concurrently count numeric data than a mutex.

Sorry, I wasn't clear at all, i meant why use the Go 1.19 atomic.Int64 over what we currently have in go 1.17/1.18.

I think we need to make it very clear (low context) why the change/comment is being made (why it's worth considering -- i.e. explain why the change would be better than what it is now OR link to a go article/blog/docs which demonstrates the benefits).

[inancgumus]

Sorry, I wasn't clear at all, i meant why use the Go 1.19 atomic.Int64 over what we currently have in go 1.17/1.18.

No worries. You can read more about it here.

The sync/atomic package defines new atomic types Bool, Int32, Int64, Uint32, Uint64, Uintptr, and Pointer. These types hide the underlying values so that all accesses are forced to use the atomic APIs. Pointer also avoids the need to convert to unsafe.Pointer at call sites. Int64 and Uint64 are automatically aligned to 64-bit boundaries in structs and allocated data, even on 32-bit systems.

IMHO, the most important benefit is this: "These (new) types hide the underlying values so that all accesses are forced to use the atomic APIs."

[inancgumus]

Changed Go 1.19 atomic.Int64 to Go 1.18's atomics (AddInt64 and LoadInt64).

[ankur22]

Thanks @inancgumus for the suggestions and links to the resources!

I've cherry-picked the commits (all but the atomic suggestion):

1. 28f4d56 -- ✅
2. 4f50651 -- The go 1.19 change is great, and it will help mitigate the risk of not using Add and Load. For this test scenario, i think a mutex is what is needed so that the counter is incremented and also read in the Fprintf in one "atomic transaction".
3. 5d2bff2 -- It does make the test a little neater, although i still prefer inline, but i'll go with it 🙂 One thing to note is that IMO having c-style var block is unnecessary in modern languages and we can now declare them closer to where they are used, which I find helps when reading the code. I guess it's one of those "it depends" situations though.

[inancgumus]

Served its purpose (#578).