Skip to content

CA-394882,CA-394883: Fix task server race condition, and testcase race condition #5737

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

Merged
merged 2 commits into from
Jun 26, 2024

Conversation

edwintorok
Copy link
Contributor

No description provided.

The code in 'next_task_id' has a race condition:

```
(* thread 1 *)
42 = !counter (* fine, integer ref operations are atomic on OCaml 4 *)
result = string_of_int (* allocates, may process pending signals and switch to another OCaml thread! *)
(* thread 2 *)
42 = !counter
result = "42"
incr counter (* fine, this is atomic on OCaml 4 *)
result (* thread 2 got task id 42 *)
(* thread 1 *)
incr counter (* fine, this is atomic on OCaml 4 *)
result (* thread 1 got same task id as thread 2: 42! *)
```

The solution is to:
* use mutexes,
* rearrange the order of operations to avoid allocations in the critical section
* use Atomic.fetch_and_add (also makes this future proof for OCaml 5)

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
The previous unit test step has created some tasks, and deleted them when completed.
However it didn't look at events.
The next step was waiting for some newly created tasks to complete and looking at events.
But it was processing all events, even those that it wasn't watching for.
These events may have referred to tasks that were already deleted.

Fix the code to only query the state of the tasks that we are waiting for
(if these go missing, it is a bug).

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
@edwintorok edwintorok merged commit 2570a99 into xapi-project:master Jun 26, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants