~gioverse/chat

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
4 2

[PATCH] scheduler: define documented types for Future's input

Details
Message ID
<20220621192414.30058-1-andrew.thorp.dev@gmail.com>
DKIM signature
missing
Download raw message
Patch: +12 -1
This commit adds WorkFunc and AndThenFunc generic types which are used
as input to Future(). This allows more explicit and nuanced
documentation for using Future() without having a novel of docs for a
single function. For example: specifying that the WorkFunc doesn't block
the event loop but AndThenFunc does is a useful detail but isn't as
relevant to the Future() function as much as it is to the closures
themselves.

Signed-off-by: Andrew Thorp <andrew.thorp.dev@gmail.com>
---
 scheduler/scheduler.go | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/scheduler/scheduler.go b/scheduler/scheduler.go
index f6a4dde..55e38ed 100644
--- a/scheduler/scheduler.go
+++ b/scheduler/scheduler.go
@@ -69,12 +69,23 @@ type futureEvent struct {
	andThen func(interface{}, error)
}

// WorkFunc is a closure that is used by Future() to execute work asynchronously outside
// of the standard event loop. It does not block the event loop so it can be used for
// more complex and time-consuming work, such as external API calls and database queries.
// Its output is passed as input to an AndThenFunc.
type WorkFunc[T any] func() (T, error)

// AndThenFunc is a closure that is used by Future() to handle the output of the Future's
// WorkFunc. It is run in response to the futureEvent on the bus and therefore blocks the
// event loop.
type AndThenFunc[T any] func(T, error)

// Future schedules work asynchronously and then invokes andThen synchronously
// on the event loop (when conn.RunFutures is invoked). This enables andThen to safely
// modify UI state because it cannot race against layout code. The most common
// use for this is to execute a query to a persistent storage API in work, then
// attach the results to the UI in andThen.
func Future[T any](conn Connection, work func() (T, error), andThen func(T, error)) {
func Future[T any](conn Connection, work WorkFunc[T], andThen AndThenFunc[T]) {
	conn.future(func() (any, error) {
		t, e := work()
		return t, e
-- 
2.33.0
Details
Message ID
<B46F5C92-DBAE-425E-9033-11ABC329F9D8@getmailspring.com>
In-Reply-To
<20220621192414.30058-1-andrew.thorp.dev@gmail.com> (view parent)
DKIM signature
missing
Download raw message
Hi Andrew, I like this!

I wonder if we can take this further and push the function definitions
into the type constraints. I've read that this is ideal in the current
implementation of generics, since it provides more opportunity for
in-lining.

    func Future[T any, Work WorkFunc[T], Then AndThenFunc[T]](c
Connection, w Work, th Then) 

I'm not even sure that snippet is valid, but you know what I mean. 

Perhaps we can also drop the `And` prefix, `ThenFunc` seems plenty
fine to me.

On Jun 22 2022, at 3:24 am, Andrew Thorp <andrew.thorp.dev@gmail.com> wrote:

> This commit adds WorkFunc and AndThenFunc generic types which are used
> as input to Future(). This allows more explicit and nuanced
> documentation for using Future() without having a novel of docs for a
> single function. For example: specifying that the WorkFunc doesn't block
> the event loop but AndThenFunc does is a useful detail but isn't as
> relevant to the Future() function as much as it is to the closures
> themselves.
> 
> Signed-off-by: Andrew Thorp <andrew.thorp.dev@gmail.com>
> ---
> scheduler/scheduler.go | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/scheduler/scheduler.go b/scheduler/scheduler.go
> index f6a4dde..55e38ed 100644
> --- a/scheduler/scheduler.go
> +++ b/scheduler/scheduler.go
> @@ -69,12 +69,23 @@ type futureEvent struct {
> 	andThen func(interface{}, error)
> }
> 
> +// WorkFunc is a closure that is used by Future() to execute work
> asynchronously outside
> +// of the standard event loop. It does not block the event loop so it
> can be used for
> +// more complex and time-consuming work, such as external API calls
> and database queries.
> +// Its output is passed as input to an AndThenFunc.
> +type WorkFunc[T any] func() (T, error)
> +
> +// AndThenFunc is a closure that is used by Future() to handle the
> output of the Future's
> +// WorkFunc. It is run in response to the futureEvent on the bus and
> therefore blocks the
> +// event loop.
> +type AndThenFunc[T any] func(T, error)
> +
> // Future schedules work asynchronously and then invokes andThen synchronously
> // on the event loop (when conn.RunFutures is invoked). This enables
> andThen to safely
> // modify UI state because it cannot race against layout code. The
> most common
> // use for this is to execute a query to a persistent storage API in
> work, then
> // attach the results to the UI in andThen.
> -func Future[T any](conn Connection, work func() (T, error), andThen
> func(T, error)) {
> +func Future[T any](conn Connection, work WorkFunc[T], andThen 
> AndThenFunc[T]) {
> 	conn.future(func() (any, error) {
> 		t, e := work()
> 		return t, e
> -- 
> 2.33.0
> 
> 
Details
Message ID
<CAFcc3FSBdkKJt6HsoT=iPpGBgR8-hpqhgVhFE-3JVyYeZOwmmQ@mail.gmail.com>
In-Reply-To
<B46F5C92-DBAE-425E-9033-11ABC329F9D8@getmailspring.com> (view parent)
DKIM signature
missing
Download raw message
> Hi Andrew, I like this!
>
> I wonder if we can take this further and push the function definitions
> into the type constraints. I've read that this is ideal in the current
> implementation of generics, since it provides more opportunity for
> in-lining.
>
>     func Future[T any, Work WorkFunc[T], Then AndThenFunc[T]](c
> Connection, w Work, th Then)
>
> I'm not even sure that snippet is valid, but you know what I mean.

I think the signature becomes really nightmarish to read if we do
this. We're also
forced to declare those extra type parameters as interface type sets, which is
a ton of extra code. See this example:

https://go.dev/play/p/k6fBYD8_GBz

I'd prefer not to push that into the type parameters. Anything running
on the bus
isn't an application's hottest path, and we're talking about the cost
of dispatching
two function calls. If you're scheduling futures in a tight loop,
you're probably using
the wrong construct.

> Perhaps we can also drop the `And` prefix, `ThenFunc` seems plenty
> fine to me.

I suppose this makes sense. I thought about requiring a struct as input:

scheduler.Future(p.Conn, Future{
    Work: workfunc,
    Then: handleWork,
})

This basically just has the upside of labelling the two closures where
you provide them.
I can't decide whether I think the readability improvement is worth
the ergonomic cost
of the extra typing though.

Cheers,
Chris
Details
Message ID
<25101CB0-9BEB-4687-A802-7DDD5962887D@getmailspring.com>
In-Reply-To
<CAFcc3FSBdkKJt6HsoT=iPpGBgR8-hpqhgVhFE-3JVyYeZOwmmQ@mail.gmail.com> (view parent)
DKIM signature
missing
Download raw message
@Chris You raise good points, and I ultimately I agree with your conclusion.
Let's leave it then! Though I would still like to drop the "And" part.
;)
Details
Message ID
<CAFcc3FTrByLwvqJ=rZkfNTUyo7+k-c7gqcdABTZTW1XO5+dfsA@mail.gmail.com>
In-Reply-To
<25101CB0-9BEB-4687-A802-7DDD5962887D@getmailspring.com> (view parent)
DKIM signature
missing
Download raw message
> @Chris You raise good points, and I ultimately I agree with your conclusion.
> Let's leave it then! Though I would still like to drop the "And" part.
> ;)

Cool, I've pushed the results of this discussion. Thanks all!
Reply to thread Export thread (mbox)