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

Grow output buffer when needed when reporting test outcomes (JS/Native) #725

Merged
merged 3 commits into from
Jan 25, 2024

Conversation

kubukoz
Copy link
Member

@kubukoz kubukoz commented Jan 24, 2024

Closes #724.

@kubukoz kubukoz marked this pull request as ready for review January 24, 2024 21:51
Comment on lines +210 to +211
val baos = new ByteArrayOutputStream(2048)
val dos = new DataOutputStream(baos)
Copy link
Member Author

@kubukoz kubukoz Jan 24, 2024

Choose a reason for hiding this comment

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

thought: two words about this:

  • 2048 is now the initial length of the array, but BAOS makes it grow when needed
  • I tried to switch to StringBuilder as suggested, but in order to keep the reader working, the output should have constant-length encoding of numbers, and it seemed bothersome to try and pad them to make it work. Instead, DataOutputStream encodes the numbers in binary as expected.
  • I'm not super comfortable with this encoding even in its current shape, because it assumes the bytes can be safely decoded from UTF-8 (new String) and encoded again (.getBytes()) without data loss, and I don't think that's generally true.

For example, this prints false:

val bb = ByteBuffer.allocate(20)
bb.putInt(42)
bb.putDouble(21.37)
val result = bb.array()
println(new String(result).getBytes().toList == result.toList)

so... I think ultimately the right solution would indeed be to use StringBuilder, albeit with a different encoding.

Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, miraculously our doubles never have the floating point part (.toMillis.toDouble). However, even 50 000 ms (50 seconds) is enough to fail the roundrip.

Copy link
Member Author

@kubukoz kubukoz Jan 24, 2024

Choose a reason for hiding this comment

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

ok, added some tests - 206de20 - for the time being, encoding the bytes in base64 to bypass the UTF-8 problem, just to have a green build. Maybe it's fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

finally, living proof of the roundtrip problem:

package demo

import cats.effect.IO
import weaver._
import concurrent.duration._

object DemoTest extends SimpleIOSuite {

  test("this should fail rather than hang") {
    IO.unit.as(failure("verbosity goes here"))
  }
}

object DemoTest2 extends SimpleIOSuite {

  test("this should fail rather than hang") {
    IO.unit.as(failure("verbosity goes here"))
  }
}

0.8.3:

image

another run:

image

206de20 :

image

@Baccata Baccata merged commit bc2f844 into disneystreaming:main Jan 25, 2024
14 checks passed
@kubukoz kubukoz deleted the js-hang-buffer-overflow branch January 25, 2024 17:27
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.

Failed IOs hang the runner on JS
2 participants