Fix hang in `KqueueExecutorScheduler` #65

LeeTibbert · 2022-09-16T22:25:53Z

Fix #64
KqueueExecutorScheduler#poll no longer hangs when there is no work to do.
The fix was actually to check for the existence of callbacks, rather than of
new events.

Manually tested on macOS M1, Monterrey 12.6 with test-cond-in-development
for Issue #52. I will be exercising this fix as I continue to work on Issue #52.

armanbilge · 2022-09-16T22:54:31Z

core/src/main/scala/epollcat/unsafe/KqueueExecutorScheduler.scala

+      if (!noCallbacks) {
+        val timeoutSpec =
+          if (timeoutIsInfinite || timeoutIsZero) null
+          else {


What happens if there are no callbacks, but there is 0 < timeout < infinity? In that case poll should still sleep for the requested time. This is not necessary for correctness, but it is important for performance (if we don't sleep, we are busy-looping). That is why the test suite currently doesn't catch this.

Thank you for the timely review and tag-team development.

re: 0 < timeout < infinity & busy looping

Isn't prevention of busy looping the responsibility of the caller? I do not know
that scheduler code. What is the usually timeout? Zero, something else.

The hang that I saw was large for a scheduler. I know I let it run two or
three minutes whilst I did code fixes in another window. In a debug
branch I can put some code in to see if it is 0 (which can mean either
return immediately (do not wait), or wait-forever) or a largish number.

I thought that poll() was supposed to minimize wait and return to
the caller, so the caller could schedule something else.

Yes, perhaps the documented contract for poll needs to be clarified.

I thought that poll() was supposed to minimize wait and return to
the caller, so the caller could schedule something else.

When the caller invokes poll(...) with a timeout of x seconds, it means that the caller has no work scheduled for x seconds and thus it is fully delegating to poll(...). If poll returns before timeout (and no new work was scheduled due to an I/O event), then the caller will simply invoke poll(...) again for the remaining time.

The event-loop algorithm is implemented here.
https://github.com/typelevel/cats-effect/blob/7ca03db50342773a79a01ecf137d953408ac6b1d/core/native/src/main/scala/cats/effect/unsafe/PollingExecutorScheduler.scala#L77-L123

That info helps. The poll is being called with INFINITE timeout, 2 events change, and 1 callback.
There was a open & close which came and went before poll() got called. I suspect the math
is not working out and an extra event-change is sneaking in.

I am going to move this to WIP, do more tracing, and read the URL you posted.
I'll post the provoking code in the issue.

You are right. I believe your general approach is correct, we just need to adjust the conditional to account for the case when 0 < timeout < infinity in which case it should proceed with the kevent64() call.

armanbilge · 2022-09-17T00:55:05Z

I was able to replicate this in a very simple test. Give it a try, and if it reproduces the issue for you as well, I propose we add it to the test suite.

  test("immediately closing a socket does not hang") {
    IOSocketChannel.open.use_
  }

LeeTibbert · 2022-09-17T17:23:35Z

Skipping over for the moment, but not ignoring. Please see next reply.

when 0 < timeout < infinity in which case it should proceed with the kevent64() call.

Thank you for the succinct test. I added it to TcpSuite.scala and hand tested
on all four cells of (Linux x macOS) x (JVM x Native). As expected macOS Native
failed with this code and mainline kexec. I then manually ran in the context of this WIP
and all four cells passed. CI confirms that.

I had gotten 90% of the way towards submitting the TcpSuite.scala now in this
WIP to mainline when I realized that it would hang mainline CI beyond the 20 second
test timeout, Duh!

LeeTibbert · 2022-09-17T17:29:36Z

when 0 < timeout < infinity in which case it should proceed with the kevent64() call.

To synchronize:

Should I proceed with this WIP and add the code which you suggested, still as a WIP?

Should I put this on the backburner, or out to feed the dog because another approach
is better or because we should just plain wait for experience & better understanding?

Please advise. I am invested in overall progress, not any one particular WIP/PR.

Given what you have said, I think I can work around this for the "IPv6 initialization" work
by "Just DON'T do that!".

I will read the "event-loop algorithm" URL, hopefully this weekend.

armanbilge · 2022-09-17T17:36:08Z

This is a legitimate issue, I'd like to fix it and get a patch out. If you don't mind I'll take the steering wheel on this one since it's interacting with the event loop. Do you mind if I push to your branch or shall I open a new PR?

armanbilge · 2022-09-17T17:36:51Z

Btw thanks for manually testing against the matrix, that was really helpful. Always good to have a test in place.

LeeTibbert · 2022-09-17T18:28:28Z

This is a legitimate issue, I'd like to fix it and get a patch out. If you don't mind I'll take the steering wheel
on this one since it's interacting with the event loop.

Understood & agreed, you have the conn (phew!). Time for the A team.

I will end this recursive edit and return to "IPv6 initialization".

Do you mind if I push to your branch or shall I open a new PR?

Since this is WIP and scratch work, I will stop pushing to it and hand it over to you.
That should make your startup easier.

We probably concur that the final submission probably wants to be a "clean" PR.

LeeTibbert · 2022-09-17T18:33:18Z

A "grasshopper" question, if you know of the top of your head. Not worth you spending time else-wise.
Thank you.

Where do I find the documentation for ".use" and ".use_". I have been searching the "cats effect 3.0" and "fs2"
documentation with no success. I spent a few hours figuring that the run through the documentation would
teach me things.

You had given me the location for ".async_". I stumbled across it today in plain sight and bold as brass in
the "Cats Effect" 3.3 documentation.

LeeTibbert · 2022-09-17T18:34:56Z

I added the test for ServerSocket because it causes a listen() call which might/complicate
the paths. This was before I read you friendly "please pass-the-baton" message.

armanbilge · 2022-09-17T18:41:12Z

Where do I find the documentation for ".use" and ".use_". I have been searching the "cats effect 3.0" and "fs2"
documentation with no success. I spent a few hours figuring that the run through the documentation would
teach me things.

They are methods on Resource. They are somewhat equivalent to the Java "try-with-resources" or Scala Using.

https://github.com/typelevel/cats-effect/blob/f12739b6af9173e8ca8ddaa35fb66f48bedde44f/kernel/shared/src/main/scala/cats/effect/kernel/Resource.scala#L197-L207

https://github.com/typelevel/cats-effect/blob/f12739b6af9173e8ca8ddaa35fb66f48bedde44f/kernel/shared/src/main/scala/cats/effect/kernel/Resource.scala#L226-L229

I added the test for ServerSocket because it causes a listen() call which might/complicate
the paths. This was before I read you friendly "please pass-the-baton" message.

No problem, great addition!

We probably concur that the final submission probably wants to be a "clean" PR.

Eh, I don't really care. I think it's good to preserve work-product so we can retrace our steps more easily.

LeeTibbert · 2022-09-17T19:55:46Z

re: kernel/Resource.scala URLs

Thank you. That is what I needed to get unstuck. Helps a number of pieces
(including evalTap()) fall into place. That file seems very well commented
(and the basis for other doc), so I will have to trace and study it. Homework on
a weekend, grumble (but not as much of a grumble as not knowing what
I am tracing ;-))

This reverts commit 61d2427.

armanbilge

😏

LeeTibbert · 2022-09-19T14:44:15Z

Arman, Thank you for sorting this out, and so quickly.

armanbilge · 2022-09-19T14:46:14Z

Thank you for catching this bug and your initial investigation! I'm glad we could get a patch out.

Fix armanbilge#64: Remove a KqExec poll hang

61d2427

LeeTibbert requested a review from armanbilge September 16, 2022 22:26

armanbilge reviewed Sep 16, 2022

View reviewed changes

LeeTibbert changed the title ~~Fix #64: Remove a KqExec poll hang~~ WIP: Fix #64: Remove a KqExec poll hang Sep 16, 2022

Add Arman's socket close no-hang test

fa70f81

sockets & server_sockets travel in pairs; their test must also

1c4a0fa

armanbilge added 3 commits September 18, 2022 13:44

Revert "Fix armanbilge#64: Remove a KqExec poll hang"

63838ca

This reverts commit 61d2427.

Add a note about the failure mode

a9f368f

Use the final kqueue change count in condition

391a739

armanbilge changed the title ~~WIP: Fix #64: Remove a KqExec poll hang~~ Fix hang in KqueueExecutorScheduler Sep 18, 2022

armanbilge approved these changes Sep 18, 2022

View reviewed changes

armanbilge merged commit b461ee1 into armanbilge:main Sep 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix hang in `KqueueExecutorScheduler` #65

Fix hang in `KqueueExecutorScheduler` #65

LeeTibbert commented Sep 16, 2022

armanbilge Sep 16, 2022

LeeTibbert Sep 16, 2022

armanbilge Sep 16, 2022

armanbilge Sep 16, 2022

LeeTibbert Sep 16, 2022

armanbilge Sep 17, 2022

armanbilge commented Sep 17, 2022

LeeTibbert commented Sep 17, 2022

LeeTibbert commented Sep 17, 2022

armanbilge commented Sep 17, 2022

armanbilge commented Sep 17, 2022

LeeTibbert commented Sep 17, 2022

LeeTibbert commented Sep 17, 2022

LeeTibbert commented Sep 17, 2022

armanbilge commented Sep 17, 2022

LeeTibbert commented Sep 17, 2022

armanbilge left a comment

LeeTibbert commented Sep 19, 2022

armanbilge commented Sep 19, 2022

Fix hang in KqueueExecutorScheduler #65

Fix hang in KqueueExecutorScheduler #65

Conversation

LeeTibbert commented Sep 16, 2022

armanbilge Sep 16, 2022

Choose a reason for hiding this comment

LeeTibbert Sep 16, 2022

Choose a reason for hiding this comment

armanbilge Sep 16, 2022

Choose a reason for hiding this comment

armanbilge Sep 16, 2022

Choose a reason for hiding this comment

LeeTibbert Sep 16, 2022

Choose a reason for hiding this comment

armanbilge Sep 17, 2022

Choose a reason for hiding this comment

armanbilge commented Sep 17, 2022

LeeTibbert commented Sep 17, 2022

LeeTibbert commented Sep 17, 2022

armanbilge commented Sep 17, 2022

armanbilge commented Sep 17, 2022

LeeTibbert commented Sep 17, 2022

LeeTibbert commented Sep 17, 2022

LeeTibbert commented Sep 17, 2022

armanbilge commented Sep 17, 2022

LeeTibbert commented Sep 17, 2022

armanbilge left a comment

Choose a reason for hiding this comment

LeeTibbert commented Sep 19, 2022

armanbilge commented Sep 19, 2022

Fix hang in `KqueueExecutorScheduler` #65

Fix hang in `KqueueExecutorScheduler` #65