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

fix: wait until the plugin update are finished #1037

Merged
merged 1 commit into from Sep 14, 2021

Conversation

yacchi
Copy link
Contributor

@yacchi yacchi commented Sep 3, 2021

Summary

In asdf plugin update --all, wait is not working and the command exits before the update of each plugin are finished.

Executing while after a pipe will run it in a subshell, so you can't use wait on the caller to wait for each command to finish.

In such a case, Process Substitution is often used, but it seems to be prohibited in this project.
However, for the usage here, I think Here string is fine.

@yacchi yacchi requested a review from a team as a code owner September 3, 2021 18:47
@Stratus3D
Copy link
Member

Thanks for the PR!

Is there any way we can write a test for this fix to verify we don't introduce this bug at a later time?

@yacchi
Copy link
Contributor Author

yacchi commented Sep 5, 2021

@Stratus3D

I'm currently writing tests in bats, and I found that bats wait for all commands to finish in a different way.
So even if there is a problem with the use of wait, the tests with bats always seem to succeed.

I haven't found a way to work around this problem, do you know of any good information?

If you write such a test code and debug output along with the date and time, you will see that the waiting areas are different.
(To make the example easier to understand, I have added sleep 2 to update_plugin.)

Added test code in plugin_update_command.bats (echo is a debug print for bats)

@test "asdf plugin-update for all plugins" {
  install_mock_plugin_repo "dummy2"
  run asdf plugin add "dummy2" "${BASE_DIR}/repo-dummy2"
  run asdf plugin-update --all
  echo "# end $(date -u) in $(basename "${BASH_SOURCE[0]}:$LINENO")" >&3
  {
    [[ "$output" = *"Updating dummy to master"* ]]
    [[ "$output" = *"Updating dummy2 to master"* ]]
  } || (
    echo "output = ${output}"
    false
  )
}

Add debug output to the code before modification in plugin_update_command.bats:

  if [ "$plugin_name" = "--all" ]; then
    if [ -d "$(asdf_data_dir)"/plugins ]; then
      echo "# update start $(date -u) in $(basename "${BASH_SOURCE[0]}:$LINENO")" >&3
      find "$(asdf_data_dir)"/plugins -mindepth 1 -maxdepth 1 -type d | while IFS= read -r dir; do
        update_plugin "$(basename "$dir")" "$dir" "$gitref" &
      done
      echo "# wait start $(date -u) in $(basename "${BASH_SOURCE[0]}:$LINENO")" >&3
      wait
      echo "# wait end $(date -u) in $(basename "${BASH_SOURCE[0]}:$LINENO")" >&3
    fi
  else

Output:

> env LANG=C bats test/plugin_update_command.bats -f "all plugins"
    update start Sun Sep  5 23:30:32 UTC 2021 in command-plugin-update.bash:14                 1/1
   wait start Sun Sep  5 23:30:32 UTC 2021 in command-plugin-update.bash:18
   wait end Sun Sep  5 23:30:32 UTC 2021 in command-plugin-update.bash:20
   end Sun Sep  5 23:30:34 UTC 2021 in bats.60856.src:120
 ✓ asdf plugin-update for all plugins

1 test, 0 failures

Add debug output to the modified code:

  if [ "$plugin_name" = "--all" ]; then
    if [ -d "$(asdf_data_dir)"/plugins ]; then
      echo "# update start $(date -u) in $(basename "${BASH_SOURCE[0]}:$LINENO")" >&3
      local plugins
      plugins=$(find "$(asdf_data_dir)"/plugins -mindepth 1 -maxdepth 1 -type d)
      while IFS= read -r dir; do
        update_plugin "$(basename "$dir")" "$dir" "$gitref" &
      done <<<"$plugins"
      echo "# wait start $(date -u) in $(basename "${BASH_SOURCE[0]}:$LINENO")" >&3
      wait
      echo "# wait end $(date -u) in $(basename "${BASH_SOURCE[0]}:$LINENO")" >&3
    fi
  else

Output:

> env LANG=C bats test/plugin_update_command.bats -f "all plugins"
    update start Sun Sep  5 23:38:39 UTC 2021 in command-plugin-update.bash:14                 1/1
   wait start Sun Sep  5 23:38:39 UTC 2021 in command-plugin-update.bash:20
   wait end Sun Sep  5 23:38:41 UTC 2021 in command-plugin-update.bash:22
   end update Sun Sep  5 23:38:41 UTC 2021 in bats.63388.src:120
 ✓ asdf plugin-update for all plugins

1 test, 0 failures

@Stratus3D
Copy link
Member

Thanks for the detailed examples @yacchi! Would it not be possible to do something like this to check if the updates are finished?

[[ "$(jobs | head | cut -f4 -d' ')" -eq "running" ]

I'm pretty sure this would not be a fool proof way of checking - the job could always finish after the command exited but before than line of test code ran. But it might be better than nothing if it works.

@yacchi yacchi force-pushed the fix-wait-plugin-update-finished branch from 77a7ada to 6ce7724 Compare September 12, 2021 00:21
@yacchi
Copy link
Contributor Author

yacchi commented Sep 12, 2021

@Stratus3D
Thank you for the test example.
I have examined various methods and added the test code.

I found out that the processes that couldn't wait and remained had a Parent PID of 1
This is because the parent process (asdf plugin-update --all) exits while the update job for each plugin is running in the background.

The jobs command was not able to detect this, but I was able to detect the remaining processes by using the ps command instead.

I'm pretty sure this would not be a fool proof way of checking - the job could always finish after the command exited but before than line of test code ran. But it might be better than nothing if it works.

It is true that this method cannot detect the problem if all the jobs are terminated before the end of the process list.
However, as far as I have tried, the code before the fix is always an error, so I think it works reasonably well.

@Stratus3D Stratus3D merged commit 7e1f2a0 into asdf-vm:master Sep 14, 2021
@Stratus3D
Copy link
Member

Thanks for the test!

@yacchi yacchi deleted the fix-wait-plugin-update-finished branch September 18, 2021 14:35
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.

None yet

2 participants