Skip to content

Add ownPoller #4133

Merged
armanbilge merged 6 commits into
typelevel:series/3.xfrom
armanbilge:issue/4009
Oct 29, 2024
Merged

Add ownPoller #4133
armanbilge merged 6 commits into
typelevel:series/3.xfrom
armanbilge:issue/4009

Conversation

@armanbilge

Copy link
Copy Markdown
Member

This PR introduces a mechanism for checking if a given poller is "owned" by the current thread and thus is safe to interact with. This is useful primarily for cancelation, which may be requested from any thread, but can happy-path additional cleanup operations when on the owning thread.

The existing accessPoller and new ownPoller methods are now bundled up in a PollerProvider API.

Finally, SelectorSystem is improved to use ownPoller to attempt to remove the linked list node for a canceled selection operation, instead of simply clearing it.

Closes #4009.

@armanbilge armanbilge added this to the v3.6.0 milestone Sep 10, 2024
durban
durban previously approved these changes Sep 23, 2024

@durban durban left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

def accessPoller(cb: P => Unit): Unit

/**
* Returns `true` if it is safe to interact with this `Poller`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we add a note that it's always safe to return false here? So in other words, we tolerate false negatives, but we cannot tolerate false positives.

var callback: Either[Throwable, Int] => Unit,
var next: CallbackNode
)
private final class Callbacks {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We've built so many of these…

}

private object PollingSystem {
trait PollerProvider[P] {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What about PollerContext or something in that vein? Kinda sucks that everything is "poller this" and "poller that". Could also be slightly more generic like WorkerContext or TLContext. ReferenceFrame is not inaccurate but more obscure.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Renamed to PollingContext

def ownPoller(poller: P): Boolean
}

private[unsafe] trait UnsealedPollingContext[P] extends PollingContext[P]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Before you tell me that private[unsafe] is the same as private since we are already in package unsafe, it's not:

private trait UnsealedPollingContext escapes its defining scope as part of type cats.effect.unsafe.UnsealedPollingContext[P]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks Scala!

@armanbilge armanbilge closed this Oct 27, 2024
@armanbilge armanbilge reopened this Oct 27, 2024
@armanbilge armanbilge closed this Oct 29, 2024
@armanbilge armanbilge reopened this Oct 29, 2024
@armanbilge
armanbilge requested a review from djspiewak October 29, 2024 06:10
djspiewak
djspiewak previously approved these changes Oct 29, 2024
@armanbilge
armanbilge merged commit ef28cf9 into typelevel:series/3.x Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add API to polling system to attempt to get current poller without shifting

3 participants