instedd / surveda

InSTEDD Surveda
https://instedd.org/technologies/surveda-mobile-surveys/
GNU General Public License v3.0
17 stars 6 forks source link

Fix: dont cache channel in queue (channel broker) #2280

Closed ysbaddaden closed 1 year ago

ysbaddaden commented 1 year ago

The individual commits show some of my failed attempts to fix the issue and keep the test suite happy. Some attempts are missing (e.g. database cleaner approach).

I had to fully remove any reference to Ask.Runtime.Channel from outside of Ask.Runtime.ChannelBroker to get the test suite happy without introducing race conditions. The channel broker is now fully responsible for creating Ask.Runtime.Channel instances and refreshing the oauth token before it expires.

The Ask.Runtime.Channel instance is also loaded before spawning the ChannelBroker processes to keep the test suite happy and avoid a deadlock situation with Ecto.Adapters.SQL.Sandbox shared mode. For example: Ask.Runtime.Session starts a DB transaction then a nested function starts a channel broker in a new process that can't checkout the DB connection because it's already been checkout by the initial process (deadlock). This is a test-only bug.

I took on the opportunity to change some calls in the channel broker to be asynchronous (GenServer.cast) instead of synchronous (GenServer.call). They don't expect any reply, and I thought at some point that it would help with my deadlock struggles (it didn't). I kept the change as I think it's good.

I leave the PR as draft because I didn't run a manual test to verify that the oauth token (I'll see that after the weekend). The PR can nonetheless be reviewed.

fixes #2279

ysbaddaden commented 1 year ago

@leandroradusky Most of the changes in channel_broker.ex is because I changed some calls (sync) to casts (async) and Elixir complained that I shall group the handle_cast and handle_call together, which led to a bunch of some noise.

The real changes in this file are: