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

fix: limit the amount of logs that launcher keeps in memory #1764

Merged
merged 1 commit into from
Jul 26, 2022

Conversation

dos65
Copy link
Collaborator

@dos65 dos65 commented Jul 21, 2022

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 dos65 force-pushed the laucnher_logs_oom branch 2 times, most recently from 3089fa4 to 1f68a5c Compare July 22, 2022 13:49
@dos65 dos65 marked this pull request as ready for review July 22, 2022 13:50
@@ -41,11 +41,8 @@ final class BspBridge(
bspServerStatus = None
}

case class RunningBspConnection(bsp: BspConnection, out: ByteArrayOutputStream) {
def logs: List[String] = {
Copy link
Contributor

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?

Copy link
Collaborator Author

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))) {

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM!

@tgodzik
Copy link
Contributor

tgodzik commented Jul 22, 2022

Actually, just realised that the test fail due to the new test: https://github.com/scalacenter/bloop/runs/7469473959?check_suite_focus=true

@dos65 dos65 force-pushed the laucnher_logs_oom branch from 1f68a5c to 7bb14cd Compare July 22, 2022 16:13
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 dos65 force-pushed the laucnher_logs_oom branch from 7bb14cd to bcbf1a5 Compare July 22, 2022 17:19
@tgodzik
Copy link
Contributor

tgodzik commented Jul 26, 2022

@dos65 I think this one only needs scalafmt and we should be good to merge!

@dos65
Copy link
Collaborator Author

dos65 commented Jul 26, 2022

@tgodzik it's workflow failure. Could you rerun it?

 Installing graalvm-ce-java11@21.1.0
  Downloading graalvm-ce-java11@21.1.0 (https://github.com/graalvm/graalvm-ce-builds/releases/download/vm-21.1.0/graalvm-ce-java11-linux-amd64-21.1.0.tar.gz) 
  Extracting /tmp/jabba-d-667052340 to /home/runner/.jabba/jdk/graalvm-ce-java11@21.1.0 
  gzip: invalid header 
  0/212
  Error: exec: Downloading graalvm-ce-java11@21.1.0 (https://github.com/graalvm/graalvm-ce-builds/releases/download/vm-21.1.0/graalvm-ce-java11-linux-amd64-21.1.0.tar.gz) 
  Extracting /tmp/jabba-d-667052340 to /home/runner/.jabba/jdk/graalvm-ce-java11@21.1.0 

@tgodzik tgodzik merged commit 15ded98 into scalacenter:main Jul 26, 2022
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.

None yet

2 participants