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

Test with monitor #79

Merged
merged 6 commits into from
Oct 29, 2022
Merged

Test with monitor #79

merged 6 commits into from
Oct 29, 2022

Conversation

lolgab
Copy link
Collaborator

@lolgab lolgab commented Oct 27, 2022

The test now pass. It can be part of the test suite if you think is valuable, otherwise we can just drop it.

P.S. Programming with IO is so much fun! I have to use it more!

println("TEST STARTS")
val promise = scala.concurrent.Promise[Unit]()
val byte = 10.toByte
scheduler.monitor(r, reads = true, writes = false)(
Copy link
Owner

Choose a reason for hiding this comment

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

Is the test or the test suite hanging? Note that the test suite will hang until all monitors are de-registered.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, in fact I had poll.stop() in the test I'm trying to port from scala-native-loop. stopping in the callback makes the test pass...

@lolgab lolgab changed the title WIP: Hanging test with monitor Test with monitor Oct 28, 2022
@lolgab lolgab requested a review from armanbilge October 28, 2022 10:27
@lolgab lolgab marked this pull request as ready for review October 28, 2022 10:27
@armanbilge
Copy link
Owner

This is fantastic! I am so happy to have a test that exercises fd monitoring without the complications of sockets, I've been wanting this for a while.

@armanbilge
Copy link
Owner

Required reading: https://lwn.net/Articles/864947/

Seems that edge-triggered polling for pipes is a bit different. This is because it was actually broken, and not possible to fully fix without breaking compatibility.

Comment on lines 35 to 44
val make: Resource[IO, Pipe] = Resource.make {
zoneResource.use { implicit zone =>
val fildes = alloc[CInt](2)
if (pipe(fildes) != 0) {
IO.raiseError(new Exception("Failed to create pipe"))
} else {
IO(new Pipe(fildes(0), fildes(1)))
}
}
}(pipe =>
Copy link
Owner

Choose a reason for hiding this comment

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

Note that calling pipe(fildes) is a side-effect and should be wrapped in IO.

Indeed, can we wrap this entire expression IO, and use a stack allocation for fildes instead of a zone?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, can we wrap this entire expression IO, and use a stack allocation for fildes instead of a zone?

Done ✅

Comment on lines 59 to 64
val readBuf = stackalloc[Byte]()
val bytesRead = read(pipe.readFd, readBuf, 1L.toULong)
assertEquals(bytesRead, 1)
assertEquals(readBuf(0), byte)
stop.run()
cb(Right(()))
Copy link
Owner

Choose a reason for hiding this comment

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

Oh, sorry, something else I just realized: if there is an assertion failure here, does it actually fail the test? Or maybe we need to capture it and return it as a cb(Left(ex))?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right! It hands if the assert fails instead of failing the test. On it!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done ✅
Now exceptions fail the test and also stops the monitoring.

Copy link
Owner

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Thank you so much for this! Excellent addition to our test suite.

@armanbilge armanbilge merged commit 64c233b into armanbilge:main Oct 29, 2022
@lolgab lolgab deleted the wip-monitor-test branch October 29, 2022 09:49
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.

2 participants