Skip to content

Comments

[contrib] Fix "custom_path" already exist bug#8770

Closed
huanmei9 wants to merge 3 commits intoapache:mainfrom
huanmei9:main
Closed

[contrib] Fix "custom_path" already exist bug#8770
huanmei9 wants to merge 3 commits intoapache:mainfrom
huanmei9:main

Conversation

@huanmei9
Copy link
Contributor

fix error when use custom_path parameter in tempdir funciton.

@Mousius
Copy link
Member

Mousius commented Aug 17, 2021

Hi @huanmei9, just curious what the use case is here? Doesn't this allow for race conditions and pollution by re-using temporary directories?

@tkonolige
Copy link
Contributor

I believe the correct way to do this is os.makedirs with exist_ok=True.

@junrushao
Copy link
Member

Hmm I agree with @tkonolige. It is slightly better to use the existing API provided by python. Please make changes accordingly and then we get the PR merged :-) Thanks a lot!

@huanmei9 huanmei9 requested a review from tqchen August 18, 2021 01:57
@huanmei9
Copy link
Contributor Author

huanmei9 commented Aug 18, 2021

Hi @tkonolige @junrushao1994 .
Thanks for your suggestions, I have changed the code according to your suggestions.

Modify it according to leandron's suggestion.

Co-authored-by: Leandro Nunes <leandro.nunes@arm.com>
@huanmei9
Copy link
Contributor Author

Hi, @leandron .
Sorry for the late reply.
My use case is to check the build fils generated by zephyr in MicroTVM, such as .map and .hex files.
In addition, regardless of the use case, the tempdir function provides a custom_path interface, so it should be robust enough.

@Mousius
Copy link
Member

Mousius commented Aug 20, 2021

Hi @huanmei9,

I would suggest that the current implementation of custom_path is robust, consider a typical task which does the following:

  1. Creates a temporary directory
  2. Writes some files
  3. Checks those files

With the current behaviour, if two tasks attempt to do this in parallel (such as parallel tests from pytest-xdist), you get the following:

  1. Task A creates a temporary directory
  2. Task A writes some files
  3. Task B attempts to create a temporary directory - raises Exception that temporary directory already exists
  4. Task A checks those files
    Task B has failed as it tries to pollute the existing temporary directory.

With this change, you can instead have:

  1. Task A creates a temporary directory
  2. Task A writes some files
  3. Task B attempts to create a temporary directory - no error raised
  4. Task B writes some files
  5. Task A checks those files - raises assertion error
  6. Task B checks those files
    Task A has now failed because of the pollution of the temporary directory and the error is also incorrectly attributed to Task A rather than Task B. This scenario only occurs intermittently and is therefore harder to debug.

Therefore, I would suggest that the current implementation of temp dir helps to prevent such concurrency bugs and raises the correct error to ensure we use a new fresh temp directory per invocation - what are your thoughts?

@huanmei9
Copy link
Contributor Author

Hi, @Mousius .
The situation you describe is indeed possible, Thanks for your suggestion.
(●'◡'●)

@huanmei9 huanmei9 closed this Aug 23, 2021
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.

6 participants