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

Set a unique java.io.tmpdir for each call to SANY #1959

Merged
merged 3 commits into from
Jul 12, 2022

Conversation

shonfeder
Copy link
Contributor

This should provide a workaround for the concurrency bug noted
tlaplus/tlaplus#688, while we wait on an upstream fix.

Note that this only allows concurrent executions of SANY in parallel processes.
AFAICT, the deeper issue noted at
#1114 (comment)
will still prevent concurrent runs of SANY within the same JVM.


@codecov-commenter
Copy link

codecov-commenter commented Jul 11, 2022

Codecov Report

Merging #1959 (b1e13e0) into unstable (2a60f81) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head b1e13e0 differs from pull request most recent head 35e7ad2. Consider uploading reports for the commit 35e7ad2 to get more accurate results

@@            Coverage Diff            @@
##           unstable    #1959   +/-   ##
=========================================
  Coverage     77.11%   77.12%           
=========================================
  Files           415      415           
  Lines         12695    12697    +2     
  Branches        570      586   +16     
=========================================
+ Hits           9790     9792    +2     
  Misses         2905     2905           
Impacted Files Coverage Δ
...ala/at/forsyte/apalache/tla/imp/SanyImporter.scala 82.81% <100.00%> (+0.55%) ⬆️
.../forsyte/apalache/tla/pp/ConstSimplifierBase.scala 96.47% <0.00%> (-1.18%) ⬇️
...a/at/forsyte/apalache/tla/lir/TlaLevelFinder.scala 100.00% <0.00%> (+2.63%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a60f81...35e7ad2. Read the comment docs.

This should provide a workaround for the concurrency bug
Copy link
Collaborator

@thpani thpani left a comment

Choose a reason for hiding this comment

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

Just want to type this out, to wrap my head around it:

If loading from string sources, this will create $TMPDIR/sanyimpXXX and write the string sources to that directory. Then it sets java.io.tmpdir to $TMPDIR/sanyimpYYY (where YYY != XXX) and calls SANY, which writes its temporary files to the latter directory.

We're not doing the first part if loading directly from files, right?

If this is all accurate, feel free to merge :)

@shonfeder
Copy link
Contributor Author

That is all accurate! One temp
dir is always created for SANY to use. And one is only created if we need to write strings into files to load from, and the two dots are different.

….scala

Co-authored-by: Thomas Pani <thomas@informal.systems>
@shonfeder shonfeder enabled auto-merge July 12, 2022 11:06
@shonfeder shonfeder merged commit 6cd7f6a into unstable Jul 12, 2022
This was referenced Jul 18, 2022
@shonfeder shonfeder deleted the shon/tmp-dir-for-sany branch July 21, 2022 21:26
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.

5 participants