Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Race condition on Process.unlink when mode is transaction #105

Closed
2 tasks done
acco opened this issue Jun 1, 2023 · 4 comments
Closed
2 tasks done

Race condition on Process.unlink when mode is transaction #105

acco opened this issue Jun 1, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@acco
Copy link
Contributor

acco commented Jun 1, 2023

Bug report

  • I confirm this is a bug with Supabase, not with my own application.
  • I confirm I have searched the Docs, GitHub Discussions, and Discord.

Describe the bug

In handle_event({:call, from}, {:client_call, bin, ready?}, data) (handler for database responses), if the client's mode is transaction and we received a ready?=true from the database, we check the db_pid back in and set it to nil.

The next time the db_pid is checked out and set in state is when the client makes another request. However, there are situations where the database will call the ClientHandler again before the client makes another request.

i.e. receiving ReadyForQuery is not a guarantee that the database won't send data until the next client request.

Currently, handle_db_pid fails when this happens, as we're trying to run Process.unlink(nil).

The solution is to ignore a nil db_pid in handle_db_pid. I think that's OK? But I don't know if getting into this state was intended behavior.

To Reproduce

What seems to be happening:

  1. Db sends a ReadyForQuery after finishing the last query.
  2. Client sends close and sync, seemingly to "wrap up" the last query. In the same bin, Client also sends parse, describe, sync - i.e., the next query.
  3. Db sends back close_complete then ReadyForQuery in its own bin.
  4. Then, it sends back the response to the subsequent query.

This is happening with the Prisma ORM client.

Expected behavior

ClientHandler does not crash when Db responds >1 time per client query.

@acco acco added the bug Something isn't working label Jun 1, 2023
@abc3
Copy link
Member

abc3 commented Jun 2, 2023

I believe that the db_handler should reset the "caller" value after ready_for_query because there can be a situation where the client is linked with another database process, and the old one will continue to send messages.

@acco
Copy link
Contributor Author

acco commented Jun 6, 2023

@abc3 I can take a stab at this with a bit more info. Are you imagining that while the Client may have "moved on" and is now associated with a new db_handler, the old db_handler - which still has messages incoming for the Client - would stay associated to the Client?

The difficulty I see is I'm not sure how we'll know that the db_handler is done responding to a Client. In a pipelined query, as I mentioned above, we have one incoming request from the Client that is going to instigate two separate db_handler responses, the first of which contains a ready_for_query.

@abc3
Copy link
Member

abc3 commented Jun 15, 2023

Sorry for the late response, it has been a tough week. According to the PostgreSQL documentation, ready_for_query indicates that the cycle is finished and the current database pool is ready for new queries.

So, after receiving this message, we can be sure that the client's query is finished. We should then clean up data.caller to avoid scenarios where DbHandler might receive an asynchronous message and forward it to the last linked client.

Would you like to implement this change?

@abc3
Copy link
Member

abc3 commented Dec 7, 2023

fixed in #207

@abc3 abc3 closed this as completed Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants