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 race condition #1445

Merged
merged 20 commits into from
Jul 13, 2023
Merged

Fix race condition #1445

merged 20 commits into from
Jul 13, 2023

Conversation

EmilyBourne
Copy link
Member

@EmilyBourne EmilyBourne commented Jul 13, 2023

The race condition is occurring on Macosx because Pyccel occasionally thinks that the source files in the stdlib folder have been modified. This then creates the following series of concurrent events:

Thread 1 Thread 2
Starts compilation process Still translating
Acquires ndarrays folder lock Still translating
Checks ndarrays folder Starts compilation process
All ok Fails to acquire ndarrays folder lock. Waiting
Releases ndarrays folder lock Waiting
Acquires locks to compile ndarrays.o (ndarrays.o.lock) Acquires ndarrays folder lock
Compilation in progress Reads contents of ndarrays folder (may see ndarrays.o.tmp file created by compilation)
Finished compilation. Released locks Checks access dates for __pyccel__/ndarrays folder and stdlib/ndarrays folder. Incorrectly determines folder needs updating
Prints wrapper file Acquires locks to copy ndarrays folder (ndarrays.h.lock, ndarrays.c.lock, ndarrays.o.lock)
Attempts to acquire locks to compile wrapper (including dependencies: ndarrays.o, (ndarrays.h, ndarrays.c) deletes all files in ndarrays folder
Waiting Copies ndarrays folder contents back (only ndarrays.h, ndarrays.c)
Waiting Releases ndarrays locks
Acquires locks Starts to check if compilation of ndarrays is necessary
Tries to compile generated file but ndarrays.o is missing Tries to acquire ndarrys.c.lock

The error is therefore not due to a race condition where 2 threads try to do the same thing, but rather due to a bad update condition. I think this is because we are checking when the files were accessed rather than when they were modified.

Additionally the compilation of ndarrays.o does not lock ndarrays.c. This is corrected in this PR but is not strictly necessary as the ndarrays.o.lock is always acquired if ndarrays.c.lock is acquired.

Commit Summary

  • Make CompileObj a context manager (with methods __enter__ and __exit__)
  • Use with compile_obj: constructs to handle locks more compactly
  • Ensure the source file is locked during compilation
  • Check the modification time not the access time to decide whether a file should be updated
  • Don't raise an error if temporary files are found in a folder being updated
  • Add detailed flags which are activated when the tests are rerun with debug logging
    image
  • Improve the definition of the min/max bounds on a test that failed. The logic now only needs touching in a single place if the bounds need narrowing

Testing
It is quite difficult to test this kind of error, however I have run the full suite of tests with assert False in the code which updates the std libraries and macosx no longer seems to go into this section of the code. (Linux triggers the assertion in the test designed to test this mechanism)

@github-actions
Copy link

github-actions bot commented Jul 13, 2023

Hello again! Thank you for this new pull request 🤩.

Don't forget to let me know when it is complete with the command /bot mark as ready.

Here is your checklist:

  • Write a clear PR description
  • Add tests to check your code works as expected
  • Update documentation if necessary
  • Update Changelog
  • Ensure any relevant issues are linked
  • Ensure new tests are passing

@EmilyBourne EmilyBourne marked this pull request as ready for review July 13, 2023 14:25
@github-actions
Copy link

github-actions bot commented Jul 13, 2023

Ran tests on commit 0e78192, for more details see here

  • ❌ docs / Documentation Format
  • 🚫 macosx / Unit tests
  • 🚫 linux / Unit tests
  • 🚫 windows / Unit tests
  • 🚫 lint / Best practices
  • ❌ spelling / Documentation spellcheck
  • ✔️ pylint / Python best practices
  • 🚫 coverage

@github-actions github-actions bot marked this pull request as draft July 13, 2023 14:30
@github-actions
Copy link

Unfortunately your PR is not passing the tests so it is not quite ready for review yet. Let me know when it is fixed with /bot mark as ready.

The failing tests are:

  • docs / Documentation Format
  • macosx / Unit tests
  • linux / Unit tests
  • windows / Unit tests
  • lint / Best practices
  • spelling / Documentation spellcheck
  • coverage

@EmilyBourne
Copy link
Member Author

/bot run spelling docs

@github-actions
Copy link

github-actions bot commented Jul 13, 2023

Ran tests on commit a893ebd, for more details see here

  • ❌ docs / Documentation Format
  • ✔️ spelling / Documentation spellcheck

@EmilyBourne EmilyBourne marked this pull request as ready for review July 13, 2023 15:10
@github-actions
Copy link

Ran tests on commit 9af773b, for more details see here

  • ❌ docs / Documentation Format
  • 🚫 macosx / Unit tests
  • 🚫 linux / Unit tests
  • ✔️ pylint / Python best practices
  • 🚫 windows / Unit tests
  • 🚫 lint / Best practices
  • ✔️ spelling / Documentation spellcheck
  • 🚫 coverage

@github-actions github-actions bot marked this pull request as draft July 13, 2023 15:15
@github-actions
Copy link

Unfortunately your PR is not passing the tests so it is not quite ready for review yet. Let me know when it is fixed with /bot mark as ready.

The failing tests are:

  • docs / Documentation Format
  • macosx / Unit tests
  • linux / Unit tests
  • windows / Unit tests
  • lint / Best practices
  • coverage

@pyccel pyccel deleted a comment from github-actions bot Jul 13, 2023
@pyccel pyccel deleted a comment from github-actions bot Jul 13, 2023
@EmilyBourne EmilyBourne marked this pull request as ready for review July 13, 2023 15:16
@github-actions
Copy link

Running tests on commit 03e00f9, for more details see here

  • ⏳ pylint
  • ⏳ coverage
  • ⏳ docs
  • ⏳ spelling
  • ⏳ macosx
  • ⏳ linux
  • ⏳ windows
  • ⏳ lint

@pyccel pyccel deleted a comment from github-actions bot Jul 13, 2023
@pyccel pyccel deleted a comment from github-actions bot Jul 13, 2023
@pyccel pyccel deleted a comment from github-actions bot Jul 13, 2023
@pyccel pyccel deleted a comment from github-actions bot Jul 13, 2023
@pyccel pyccel deleted a comment from github-actions bot Jul 13, 2023
@pyccel pyccel deleted a comment from github-actions bot Jul 13, 2023
@pyccel pyccel deleted a comment from github-actions bot Jul 13, 2023
@pyccel pyccel deleted a comment from github-actions bot Jul 13, 2023
@github-actions
Copy link

Ran tests on commit 03e00f9, for more details see here

  • ✔️ docs / Documentation Format
  • ✔️ linux / Unit tests
  • ✔️ windows / Unit tests
  • ✔️ pylint / Python best practices
  • ✔️ macosx / Unit tests
  • ✔️ lint / Best practices
  • ✔️ spelling / Documentation spellcheck
  • ❌ coverage / Unit tests

@github-actions github-actions bot marked this pull request as ready for review July 13, 2023 16:04
@github-actions
Copy link

Hey @pyccel/pyccel-dev ! @EmilyBourne has just created this great new pull request! Check it out and let me know what you think!

@EmilyBourne
Copy link
Member Author

/bot run pr_tests

@github-actions
Copy link

github-actions bot commented Jul 13, 2023

Ran tests on commit fb3b1f4, for more details see here

  • ✔️ windows / Unit tests
  • ✔️ linux / Unit tests
  • ✔️ docs / Documentation Format
  • ✔️ macosx / Unit tests
  • ✔️ lint / Best practices
  • ✔️ spelling / Documentation spellcheck
  • ✔️ pylint / Python best practices
  • ❌ coverage / Unit tests

Copy link
Member

@yguclu yguclu left a comment

Choose a reason for hiding this comment

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

I am so glad that you finally found the cause of the macOS failures!

The fix looks good and elegant to me. Great job!

I have a few minor comments.

pyccel/codegen/compiling/basic.py Outdated Show resolved Hide resolved
pyccel/codegen/compiling/compilers.py Outdated Show resolved Hide resolved
pyccel/codegen/compiling/compilers.py Outdated Show resolved Hide resolved
pyccel/codegen/utilities.py Outdated Show resolved Hide resolved
pyccel/codegen/utilities.py Outdated Show resolved Hide resolved
@github-actions github-actions bot marked this pull request as draft July 13, 2023 17:50
@github-actions
Copy link

@EmilyBourne, @yguclu has a few questions/comments about your code. Can you go through and see if you agree with them. If not go ahead and explain why. Once you've adressed all the comments let me know with /bot mark as ready and we will see if we can get approval.

@EmilyBourne
Copy link
Member Author

/bot run docs

@github-actions
Copy link

github-actions bot commented Jul 13, 2023

Ran tests on commit 892a6ad, for more details see here

  • ✔️ docs / Documentation Format

@EmilyBourne EmilyBourne marked this pull request as ready for review July 13, 2023 18:16
@github-actions
Copy link

github-actions bot commented Jul 13, 2023

Ran tests on commit 892a6ad, for more details see here

  • ✔️ linux / Unit tests
  • ✔️ macosx / Unit tests
  • ✔️ windows / Unit tests
  • ✔️ docs / Documentation Format
  • ✔️ lint / Best practices
  • ✔️ spelling / Documentation spellcheck
  • ✔️ pylint / Python best practices
  • ❌ coverage / Unit tests

@github-actions github-actions bot added the Ready_to_merge Approved by senior developer. Ready for final approval and merge label Jul 13, 2023
@yguclu yguclu merged commit 1fbb332 into devel Jul 13, 2023
38 of 39 checks passed
@yguclu yguclu deleted the ebourne_fix_race_condition branch July 13, 2023 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready_to_merge Approved by senior developer. Ready for final approval and merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants