-
Notifications
You must be signed in to change notification settings - Fork 205
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: limit the amount of logs that launcher keeps in memory #1764
Conversation
3089fa4
to
1f68a5c
Compare
@@ -41,11 +41,8 @@ final class BspBridge( | |||
bspServerStatus = None | |||
} | |||
|
|||
case class RunningBspConnection(bsp: BspConnection, out: ByteArrayOutputStream) { | |||
def logs: List[String] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, where are these logs used? Don't we send them out immediately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in BspBridge.waitForOpenBsp
There is some logic over log message -
} else if (conn.logs.exists(_.contains(BspStartLog))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Actually, just realised that the test fail due to the new test: https://github.com/scalacenter/bloop/runs/7469473959?check_suite_focus=true |
I'm noticed that this outputStream might produce an unhandled OOM error when it used from Metals (~80MB in memory). Can be reproduced on long running session with a lot of diagnostics.
@dos65 I think this one only needs scalafmt and we should be good to merge! |
@tgodzik it's workflow failure. Could you rerun it?
|
I'm noticed that this outputStream might produce an unhandled OOM error when it used from Metals (~80MB in memory).
Can be reproduced on long running session with a lot of diagnostics.