Skip to content
New issue

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

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

Already on GitHub? # to your account

fix(postgrest): race condition when setting fetchOptions and execute method call #325

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

grdsdev
Copy link
Collaborator

@grdsdev grdsdev commented Apr 5, 2024

What kind of change does this PR introduce?

Fixes a bug which could lead to a race condition when executing the request on Postgrest.

What is the current behavior?

Lock was being acquired first for updating the fetchOptions and then inside the execute method for executing the request, this could lead to the execute method to use the wrong fetch options.

What is the new behavior?

Move assignment of fetchOptions inside execute method, on a single lock acquisition.

@grdsdev grdsdev merged commit 97d1900 into main Apr 5, 2024
8 checks passed
@grdsdev grdsdev deleted the fix/postgrest branch April 5, 2024 18:46
@steve-chavez
Copy link
Member

Lock was being acquired first for updating the fetchOptions and then inside the execute method for executing the request, this could lead to the execute method to use the wrong fetch options.

@grdsdev Q: Why are locks needed on the client? Can't it be stateless? (Since PostgREST requests are stateless)

@grdsdev
Copy link
Collaborator Author

grdsdev commented Apr 5, 2024

@steve-chavez

Check the following test:

func testRaceCondition() async {
    let select = client.from("users").select()

    Task {
      print("Task 1")
      try await select.execute(options: FetchOptions(head: true))
    }

    Task {
      print("Task 2")
      try await select.execute(options: FetchOptions(head: false))
    }

    try? await Task.sleep(nanoseconds: NSEC_PER_SEC * 2)
  }

Task 1 and Task 2 can run on different threads, Task 2 can even start running before Task 1, that is all up to the Swift concurrency system.

@steve-chavez
Copy link
Member

@grdsdev Got it. Thanks for the explanation 👍

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants