Fix non-worker output and also bootstrap scalac nondeterminism #47
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
July 26, 2024 02:00
…lose the streams we open.
…e of a sandbox This fixes a bug where every bootstrap scalac action was writing class files to the same directory: tmp/classes. Each action would run scalac with the output directory set to tmp/classes and then create a jar from all the class files in that directory. If you're only running a single bootstrap scalac action, then this works fine. The bug happens when you run multiple bootstrap scalac actions because you build up class files in the tmp/classes. This causes the jars you produce to contain not only the current target's class files, but also any class files from previous bootstrap scalac actions. Which specific class files are in your jar depend on what actions have run up until that point in time. This is not deterministic. When executing with the sandboxed strategy this bug wasn't an issue because the sandbox kept actions sufficiently isolated from each other. Meaning you'd get only your action's classes in the output jar.
gregghz
reviewed
Jul 26, 2024
| // If we do something synchronous with Using, then there's a race condition where the | ||
| // streams can get closed before the Future is completed. | ||
| var outStream: ByteArrayOutputStream = null | ||
| var out: PrintStream = null |
There was a problem hiding this comment.
Why do these have to be set to null first?
There was a problem hiding this comment.
Because if I create the stream here and something goes wrong, then we could leak memory by not closing the stream. By doing so in the future, I'm trying to avoid that. It needs to be declared outside the future, so I can reference it in a subsequent future to be able to close it after the original future has done its work. That's what I'm trying to do with the andThen calls.
|
This looks good to me - that being said I will defer to Gregg's scala expertise 😁 |
gregghz
approved these changes
Jul 26, 2024
|
Thanks all for the reviews! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes anything that inherits from WorkerMain not writing to the console when not run as a worker.
Fixes a non-determinism bug with bootstrap scalac actions. Check that commit message for a more detailed explanation.