Introduce IdPool interface

[ruudk]

After debugging Skipper's goroutine leak, @adri came with the idea to completely disable the dynamic id pool for Skipper.

I think it makes sense, as Skipper doesn't need this pool because it creates and destroys clients for every request.

Therefore, this PR introduces an IdPool interface with 2 implementations: DynamicIdPool (current implementation) and a new StaticIdPool that always allocates the same number.

This could be used by projects that don't need to have an always running goroutine that generates ids (Skipper for example).

Todo

• Properly close channel Close ID pool when closing client #51
• Add new ClientFactory that creates a client with static id pool

@yookoala WDYT about this? Would this make sense?

[yookoala]

I'm not entirely convinced that this caused issue #48. But this is a nice addition to the current code base. Please rebase on the current master branch. I will merge after this.

[ruudk]

@yookoala I improved the PR a bit. Is this the right direction? I'm getting stuck with the tests, and fail to fix it properly. Could you advice me? 🙏

[yookoala]

@ruudk: The newly written dynamic id pool has some data race issue. I believe the go routine line 58 in id_pool.go needs some work to detect the ids channel's closing and simply quit. Alloc and Release should now detect the channel's closing and work differently (you cannot send to or read from closed channel or nil).

Check these links out to see if you get any inspiration:

• https://go101.org/article/channel-closing.html
• https://stackoverflow.com/questions/16105325/how-to-check-a-channel-is-closed-or-not-without-reading-it

[ruudk]

@yookoala Today I decided to give this another shot and I was able to fix the race condition on Go 1.15. Turns out that it doesn't work on Go 1.11 but I believe that's because the race condition checker has been improved since then? (turns out it fails randomly 🤯 , any idea?)

I added multiple commits because this is not 100% ready to merge yet. I would first want to get your opinion on how to solve this properly.

In 97b6027 I added a sync.WaitGroup to manage the shutdown of the pool. This works fine, except for 1 test.

I thought this might be a breaking change, so I removed it, to get the tests passing again in 4a2618a

But I feel it should be done like this: f674ebf

What do you think?

[ruudk]

I verified that this PR fixes the Skipper goroutine leak 🎉

[yookoala]

When rerun the test, the racing is gone. This is probably due to some minor difference in the parallel steps in initialization. I'll check on this more.

[ruudk]

Any update? 🙏

[yookoala]

Still testing if I can get rid of the occasional race condition. Please wait.

[yookoala]

@ruudk: #70 has introduced a simpler idPool implementation without creating an extra interface. I think this might be a simpler solution to the memory leak issue. Please go ahead and check if it really solve the issue. We can revisit this interface idea if needed.