@mizoR - Thank you! Thank you! Thank you, for finding and solving this! At Vox Pupuli, this issue has been plaguing us for months.

@ferrarimarco Can this be merged?

Thanks again!

Great PR!

Can you please provide a test for this as well? Thanks again for the contribution :)

@ferrarimarco

I tried to add test code now.

But it has not been easy for me... This is a bug that reproduces only when the response requested in the second thread arrives before the response requested in the first thread. So this is difficult to express in tests.

On the other hand, this PR is a change of GitHubChangelogGenerator::OctoFetcher#fetch_events_async and #fetch_events_async test passes. So could you consider merging without adding tests?

Or could you give me any hints if you have good idea about how to test 🙏

Anyway thanks for your reply 🙂

I think you can starting by mocking Thread.new() and work from there :)

@ferrarimarco I really doubt you can replicate race conditions with mocking threads.

I rebased my commits to resolve conflicts :) Could you check it again?

Thanks!

Should fetch_comments_async use the same logic? It also uses threads. Basically grep for MAX_THREAD_NUMBER to find where to apply the pattern.

Thanks! As far as I've grep'd, there are three functions that use threads.

• #fetch_events_async
• #fetch_comments_async
• #fetch_tag_shas_async

#fetch_events_async and #fetch_comments_async are using Octokit#last_response, so there was a race condition problem, so I fixed #fetch_comments_async as well as #fetch_events_async.

#fetch_tag_shas_async doesn't use Octokit#last_response, so I think it will be fine as is.

While I think this PR looks okay... I think creates a situation where future changes of the code may introduce thread safety issues or break assumptions about the sharing of clients.

My advice is to use a thread pool and queue jobs into it:

https://github.com/ruby-concurrency/concurrent-ruby/blob/master/docs-source/thread_pools.md

In addition, unless the client is using persistent connections under the hood, caching them probably doesn't serve much purpose.

Finally, you are trying to maximise I/O throughput by putting what appears to be HTTP/1.1 requests on different threads. If that's the case, you might want to investigate async-http which would potentially improve throughput, reduce resource utilisation and avoid the need for thread synchronisation primitives.

I had a quick look and octokit is using faraday:

https://github.com/octokit/octokit.rb/blob/4-stable/octokit.gemspec#L9

We have the async-http-faraday adapter so it should work. Just FYI.

@ioquatix while I agree that's a much cleaner solution, this currently fixes a nasty bug. Do you think you can come up with an alternative patch or should this be merged until a proper solution can be implemented?

@ekohl #784 is now ready for review and should be merged in favour of this IMHO.

OK, I agree with that too. I close this. Thanks!

@mizoR thanks for your work and bringing attention to this issue as well as a potential solution, we all learnt a lot!