fix: Concurrency issue in build plugin docker integration#2382
fix: Concurrency issue in build plugin docker integration#2382johanandren merged 2 commits intomainfrom
Conversation
| val collectedLines = Vector.newBuilder[String] | ||
| val processLogger = ProcessLogger(out => | ||
| collectedLines.synchronized { | ||
| collectedLines += out | ||
| }) | ||
| val exitCode = Process(s"docker compose -f $file config", None).!(processLogger) | ||
| val lines = collectedLines.synchronized { | ||
| collectedLines.result() | ||
| } | ||
| if (exitCode != 0) { | ||
| println("Docker compose call returned non-zero exit code. Command output: + " + lines.mkString("\n")) | ||
| } | ||
| lines |
There was a problem hiding this comment.
Could we use a StringBuffer instead?
It's thread-safe and will make the code much more straightforward.
There was a problem hiding this comment.
Ah, I see now that we don't return a String, but a Seq[String]. That's probably the reason why we didn't use StringBuffer at first place.
Anyway, we can also collect the string and then make a Seq of it.
There was a problem hiding this comment.
I see we have a different solution in the sbt build itself:
def commandOutputOrElse(command: String)(default: => String): String = {
val buffer = new StringBuffer
val io = new ProcessIO(BasicIO.input(connect = false), BasicIO.processFully(buffer), BasicIO.processFully(buffer))
if (Process(command).run(io).exitValue() == 0) buffer.toString.trim else default
}Something like that would be fine. Or this as is. I don't think the four added lines makes this that hard to follow?
There was a problem hiding this comment.
The +3 for actually looking at the process return is unrelated boycouting btw
octonato
left a comment
octonato
left a comment
There was a problem hiding this comment.
LGTM, but still think StringBuffer is better.
It's not that it's hard to follow. It's just that StringBuffer is simpler and delivers the same.
Anyway, no strong opinions on this. Feel free to merge as is.
2 participants
Saw this fail a couple of times yesterday in CI for #2365