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

job-runner: fix async interaction #2785

Merged
merged 1 commit into from
Jun 18, 2024
Merged

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Jun 6, 2024

(built on #2754)
Closes #2784

(hopefully this does it...)

job-runner: move finally block within async code

  • The job_runner is async now and runs in the background (i.e. you have to await any active tasks if you want it to be synchronous).
  • This means the finally block was run during the operation rather than after it.
  • The solution is to inline this with the async code.

@oliver-sanders oliver-sanders added this to the 2.3.0 milestone Jun 6, 2024
@oliver-sanders oliver-sanders changed the title job-runner: job-runner: fix async interaction Jun 11, 2024
@oliver-sanders oliver-sanders marked this pull request as ready for review June 12, 2024 15:37
@oliver-sanders oliver-sanders self-assigned this Jun 12, 2024
@wxtim
Copy link
Contributor

wxtim commented Jun 13, 2024

It looks reasonable, works locally for me.

You might find that something along the lines of cylc-rose/tests/functional/test_rose_stem.py might provide a pattern for setting up a fake local SVN repo to test the fileinstall on.

@oliver-sanders
Copy link
Member Author

Thanks, I just realised we already have a repo checked out that we can test, the Rose repo! Easy enough to reproduce with a local git fileinstall.

@oliver-sanders
Copy link
Member Author

(mac os tests squiffy ATM #2787)

Copy link
Contributor

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, holding off merging until #2754

* The job_runner was converted to async, its tasks run in the background
  (i.e. calls complete before the code has finished running, you cannot
  await them from synchronous code).
* This jiggered up some `finally` blocks which were being invoked too
  soon.
* This PR moves the async code up one level in the stack, brining these
  finally blocks within the lifecycle of the job runner async code
  allowing them to function correctly.
* Closes metomi#2784
@oliver-sanders
Copy link
Member Author

Rebased onto master.

@MetRonnie MetRonnie merged commit 6691b0d into metomi:master Jun 18, 2024
5 of 6 checks passed
@oliver-sanders oliver-sanders deleted the 2784 branch June 18, 2024 11: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.

Failure to install from subversion
3 participants