IEP-70: Async Continuation Executor

[Pavel Tupitsyn]

Igniters,

Please review the IEP [1] and let me know your thoughts.

[1]
https://cwiki.apache.org/confluence/display/IGNITE/IEP-70%3A+Async+Continuation+Executor

[Alexey Kukushkin]

Hi Pavel,

Extending Java async API with additional Executor parameters looks OK to me.

It is not clear from the IEP how you are going to do that for .NET async
API. My understanding is in .NET we do not add any Executors. Instead, the
Ignite Async API should use (SynchronizationContext.Current ??
TaskScheduler.Current) by default and it should have exciting behavior (use
Ignite striped pool) if ConfigureAwait(false) was specified for the Task
result.

Is my understanding correct?


пн, 15 мар. 2021 г. в 19:24, Pavel Tupitsyn <[hidden email]>:

> Igniters,
>
> Please review the IEP [1] and let me know your thoughts.
>
> [1]
>
> https://cwiki.apache.org/confluence/display/IGNITE/IEP-70%3A+Async+Continuation+Executor
>


--
Best regards,
Alexey

[Pavel Tupitsyn]

Alexey,

.NET thick API delegates to Java directly.

When you do ICache.PutAsync():
* Future is created on Java side, .listen() is called
* TaskCompletionSource is created on .NET side, its Task is returned to the
user
* Operation completes, Future listener is called on the Java side
* Listener invokes JNI callback to .NET, where
TaskCompletionSource.SetResult is called

Therefore, .NET user code (in ContinueWith or after await) will be executed
on the Java
thread that invokes the future listener.

After the proposed fix, future listeners will be invoked on
ForkJoinPool#commonPool (instead of striped pool).
So .NET continuations will end up in commonPool as well, which solves the
problem for .NET automatically, no changes required.

Does that make sense?

On Tue, Mar 16, 2021 at 1:52 PM Alexey Kukushkin <[hidden email]>
wrote:

> Hi Pavel,
>
> Extending Java async API with additional Executor parameters looks OK to
> me.
>
> It is not clear from the IEP how you are going to do that for .NET async
> API. My understanding is in .NET we do not add any Executors. Instead, the
> Ignite Async API should use (SynchronizationContext.Current ??
> TaskScheduler.Current) by default and it should have exciting behavior (use
> Ignite striped pool) if ConfigureAwait(false) was specified for the Task
> result.
>
> Is my understanding correct?
>
>
> пн, 15 мар. 2021 г. в 19:24, Pavel Tupitsyn <[hidden email]>:
>
> > Igniters,
> >
> > Please review the IEP [1] and let me know your thoughts.
> >
> > [1]
> >
> >
> https://cwiki.apache.org/confluence/display/IGNITE/IEP-70%3A+Async+Continuation+Executor
> >
>
>
> --
> Best regards,
> Alexey
>

[Igor Sapego-2]

Pavel, I like the proposal,

+1 from me

Best Regards,
Igor


On Tue, Mar 16, 2021 at 6:49 PM Pavel Tupitsyn <[hidden email]> wrote:

> Alexey,
>
> .NET thick API delegates to Java directly.
>
> When you do ICache.PutAsync():
> * Future is created on Java side, .listen() is called
> * TaskCompletionSource is created on .NET side, its Task is returned to the
> user
> * Operation completes, Future listener is called on the Java side
> * Listener invokes JNI callback to .NET, where
> TaskCompletionSource.SetResult is called
>
> Therefore, .NET user code (in ContinueWith or after await) will be executed
> on the Java
> thread that invokes the future listener.
>
> After the proposed fix, future listeners will be invoked on
> ForkJoinPool#commonPool (instead of striped pool).
> So .NET continuations will end up in commonPool as well, which solves the
> problem for .NET automatically, no changes required.
>
> Does that make sense?
>
> On Tue, Mar 16, 2021 at 1:52 PM Alexey Kukushkin <
> [hidden email]>
> wrote:
>
> > Hi Pavel,
> >
> > Extending Java async API with additional Executor parameters looks OK to
> > me.
> >
> > It is not clear from the IEP how you are going to do that for .NET async
> > API. My understanding is in .NET we do not add any Executors. Instead,
> the
> > Ignite Async API should use (SynchronizationContext.Current ??
> > TaskScheduler.Current) by default and it should have exciting behavior
> (use
> > Ignite striped pool) if ConfigureAwait(false) was specified for the Task
> > result.
> >
> > Is my understanding correct?
> >
> >
> > пн, 15 мар. 2021 г. в 19:24, Pavel Tupitsyn <[hidden email]>:
> >
> > > Igniters,
> > >
> > > Please review the IEP [1] and let me know your thoughts.
> > >
> > > [1]
> > >
> > >
> >
> https://cwiki.apache.org/confluence/display/IGNITE/IEP-70%3A+Async+Continuation+Executor
> > >
> >
> >
> > --
> > Best regards,
> > Alexey
> >
>

[Alexey Kukushkin]

Pavel,

My understanding might be wrong but I think the best practice (or even
strongly recommended way) to implement async methods in .NET is to execute
continuation on the caller's thread if ConfigureAwait(false) was not
specified. Pseudo-code might look like:

async Task PutAsync(K k, V v)
{
    var continuationExecutor = configureAwait
        ? (SynchronizationContext.Current ?? TaskScheduler.Current)
        : null;

    await <<async implementation>>

    continuationExecutor.Post(continuation);
}

I got this understanding from reading some blog
about SynchronizationContext lots of time ago. They were saying they
created SynchronizationContext specifically to allow posting continuations
to the caller's thread.

The reason for that is to simplify the user's code to avoid routing in the
code. Suppose you have a UI (like WPF or WinForms) event handler that must
be processed on the U thread:

async Task Button1_Click(EventArgs args)
{
    ignite.PutAsync(args.Key, args.Value);
    Button1.Disabled = true;
}

Executing the "Button1.Disabled = true" on a ForkJoinPool pool would cause
a "Trying to modify UI on a non-UI thread" exception. But if you
capture SynchronizationContext.Current in PutAsync and then route
continuation to the captured context then the code would work.

I think the users really expect the continuations to be executed on the
caller's thread.

Sometimes you know that your continuation is really fast and safe and you
want to avoid switching threads to improve performance. In this case you
use ConfigureAwait(false) like

ignite.PutAsync(args.Key, args.Value).ConfigureAwat(false);

In this case the continuation executes on the Ignite thread without routing
to the caller's thread.

вт, 16 мар. 2021 г. в 18:49, Pavel Tupitsyn <[hidden email]>:

> Alexey,
>
> .NET thick API delegates to Java directly.
>
> When you do ICache.PutAsync():
> * Future is created on Java side, .listen() is called
> * TaskCompletionSource is created on .NET side, its Task is returned to the
> user
> * Operation completes, Future listener is called on the Java side
> * Listener invokes JNI callback to .NET, where
> TaskCompletionSource.SetResult is called
>
> Therefore, .NET user code (in ContinueWith or after await) will be executed
> on the Java
> thread that invokes the future listener.
>
> After the proposed fix, future listeners will be invoked on
> ForkJoinPool#commonPool (instead of striped pool).
> So .NET continuations will end up in commonPool as well, which solves the
> problem for .NET automatically, no changes required.
>
> Does that make sense?
>
> On Tue, Mar 16, 2021 at 1:52 PM Alexey Kukushkin <
> [hidden email]>
> wrote:
>
> > Hi Pavel,
> >
> > Extending Java async API with additional Executor parameters looks OK to
> > me.
> >
> > It is not clear from the IEP how you are going to do that for .NET async
> > API. My understanding is in .NET we do not add any Executors. Instead,
> the
> > Ignite Async API should use (SynchronizationContext.Current ??
> > TaskScheduler.Current) by default and it should have exciting behavior
> (use
> > Ignite striped pool) if ConfigureAwait(false) was specified for the Task
> > result.
> >
> > Is my understanding correct?
> >
> >
> > пн, 15 мар. 2021 г. в 19:24, Pavel Tupitsyn <[hidden email]>:
> >
> > > Igniters,
> > >
> > > Please review the IEP [1] and let me know your thoughts.
> > >
> > > [1]
> > >
> > >
> >
> https://cwiki.apache.org/confluence/display/IGNITE/IEP-70%3A+Async+Continuation+Executor
> > >
> >
> >
> > --
> > Best regards,
> > Alexey
> >
>

--
Best regards,
Alexey

[Raymond Wilson]

There is a (long) discussion here (
https://devblogs.microsoft.com/dotnet/configureawait-faq/) regarding use of
ConfigureAwait().

Putting edge cases aside, there is a general use case for
.ConfigureAwait(true) in application UI contexts to ensure the continuation
joins a UI thread (for example), where failing to do so results in an error.

The other general use case relates more to library code where you are
typically running your tasks on the managed thread pool (assuming no custom
task schedulers etc). In this case the .library is agnostic about which
thread pool thread picks up the continuation, so forcing the continuation
to join to the original thread may be both a performance penalty from the
join overhead, but also may add latency waiting for that thread to become
available again.

I don't have good insight into how the striped thread pool is used in
Ignite, but in highly concurrent environments letting those threads escape
into the calling client context seems like a bad idea in general.

Stepping back a little, the Cache Async operations are good for when there
will be an IO operation incurred because the requested element is not
present in memory. If it is present in memory, then a Sync operation will
be more performant. Is it possible to do a two step operation like this
'Cache.GetIfInMemory() ?? await Cache.GetAsync()' to allow the calling
context to optimise the calling model dynamically?

Thanks,
Raymond.


On Wed, Mar 17, 2021 at 6:14 AM Alexey Kukushkin <[hidden email]>
wrote:

> Pavel,
>
> My understanding might be wrong but I think the best practice (or even
> strongly recommended way) to implement async methods in .NET is to execute
> continuation on the caller's thread if ConfigureAwait(false) was not
> specified. Pseudo-code might look like:
>
> async Task PutAsync(K k, V v)
> {
>     var continuationExecutor = configureAwait
>         ? (SynchronizationContext.Current ?? TaskScheduler.Current)
>         : null;
>
>     await <<async implementation>>
>
>     continuationExecutor.Post(continuation);
> }
>
> I got this understanding from reading some blog
> about SynchronizationContext lots of time ago. They were saying they
> created SynchronizationContext specifically to allow posting continuations
> to the caller's thread.
>
> The reason for that is to simplify the user's code to avoid routing in the
> code. Suppose you have a UI (like WPF or WinForms) event handler that must
> be processed on the U thread:
>
> async Task Button1_Click(EventArgs args)
> {
>     ignite.PutAsync(args.Key, args.Value);
>     Button1.Disabled = true;
> }
>
> Executing the "Button1.Disabled = true" on a ForkJoinPool pool would cause
> a "Trying to modify UI on a non-UI thread" exception. But if you
> capture SynchronizationContext.Current in PutAsync and then route
> continuation to the captured context then the code would work.
>
> I think the users really expect the continuations to be executed on the
> caller's thread.
>
> Sometimes you know that your continuation is really fast and safe and you
> want to avoid switching threads to improve performance. In this case you
> use ConfigureAwait(false) like
>
> ignite.PutAsync(args.Key, args.Value).ConfigureAwat(false);
>
> In this case the continuation executes on the Ignite thread without routing
> to the caller's thread.
>
> вт, 16 мар. 2021 г. в 18:49, Pavel Tupitsyn <[hidden email]>:
>
> > Alexey,
> >
> > .NET thick API delegates to Java directly.
> >
> > When you do ICache.PutAsync():
> > * Future is created on Java side, .listen() is called
> > * TaskCompletionSource is created on .NET side, its Task is returned to
> the
> > user
> > * Operation completes, Future listener is called on the Java side
> > * Listener invokes JNI callback to .NET, where
> > TaskCompletionSource.SetResult is called
> >
> > Therefore, .NET user code (in ContinueWith or after await) will be
> executed
> > on the Java
> > thread that invokes the future listener.
> >
> > After the proposed fix, future listeners will be invoked on
> > ForkJoinPool#commonPool (instead of striped pool).
> > So .NET continuations will end up in commonPool as well, which solves the
> > problem for .NET automatically, no changes required.
> >
> > Does that make sense?
> >
> > On Tue, Mar 16, 2021 at 1:52 PM Alexey Kukushkin <
> > [hidden email]>
> > wrote:
> >
> > > Hi Pavel,
> > >
> > > Extending Java async API with additional Executor parameters looks OK
> to
> > > me.
> > >
> > > It is not clear from the IEP how you are going to do that for .NET
> async
> > > API. My understanding is in .NET we do not add any Executors. Instead,
> > the
> > > Ignite Async API should use (SynchronizationContext.Current ??
> > > TaskScheduler.Current) by default and it should have exciting behavior
> > (use
> > > Ignite striped pool) if ConfigureAwait(false) was specified for the
> Task
> > > result.
> > >
> > > Is my understanding correct?
> > >
> > >
> > > пн, 15 мар. 2021 г. в 19:24, Pavel Tupitsyn <[hidden email]>:
> > >
> > > > Igniters,
> > > >
> > > > Please review the IEP [1] and let me know your thoughts.
> > > >
> > > > [1]
> > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/IGNITE/IEP-70%3A+Async+Continuation+Executor
> > > >
> > >
> > >
> > > --
> > > Best regards,
> > > Alexey
> > >
> >
>
>
> --
> Best regards,
> Alexey
>

--
<http://www.trimble.com/>
Raymond Wilson
Solution Architect, Civil Construction Software Systems (CCSS)
11 Birmingham Drive | Christchurch, New Zealand
[hidden email]

<https://worksos.trimble.com/?utm_source=Trimble&utm_medium=emailsign&utm_campaign=Launch>

[Alexey Kukushkin]

Raymond,

The article you referenced
<https://devblogs.microsoft.com/dotnet/configureawait-faq/> is great! It
seems to me the article rather proved what I suggested: an async operation
implementation:

   - Uses the async operation thread (Ignite's thread) if ConfigureAwait is
   false (by default it is true)
   - Uses caller's current SynchornizationContext if it is specified
   - Uses caller's TaskScheduler.Current if current
   SynchornizationContext is null

In the application code (outside framework callbacks) the
SynchornizationContext.Current will be null and TaskScheduler.Current is
the .NET's fork-join thread pool. Thus, normally the .NET thread pool would
be used for continuations as Pavel suggested in the IEP.

Executing Async operation in a context where
SynchornizationContext.Current is not null means some framework explicitly
configured the context and that means it is important to execute the
continuation in that context (like UI or xUnit main thread)

I agree that routing back to original context might result in waiting,
which is very dangerous for an Ignite thread. We can create a worker thread
to route continuation to original context.


вт, 16 мар. 2021 г. в 21:40, Raymond Wilson <[hidden email]>:

> There is a (long) discussion here (
> https://devblogs.microsoft.com/dotnet/configureawait-faq/) regarding use
> of
> ConfigureAwait().
>
> Putting edge cases aside, there is a general use case for
> .ConfigureAwait(true) in application UI contexts to ensure the continuation
> joins a UI thread (for example), where failing to do so results in an
> error.
>
> The other general use case relates more to library code where you are
> typically running your tasks on the managed thread pool (assuming no custom
> task schedulers etc). In this case the .library is agnostic about which
> thread pool thread picks up the continuation, so forcing the continuation
> to join to the original thread may be both a performance penalty from the
> join overhead, but also may add latency waiting for that thread to become
> available again.
>
> I don't have good insight into how the striped thread pool is used in
> Ignite, but in highly concurrent environments letting those threads escape
> into the calling client context seems like a bad idea in general.
>
> Stepping back a little, the Cache Async operations are good for when there
> will be an IO operation incurred because the requested element is not
> present in memory. If it is present in memory, then a Sync operation will
> be more performant. Is it possible to do a two step operation like this
> 'Cache.GetIfInMemory() ?? await Cache.GetAsync()' to allow the calling
> context to optimise the calling model dynamically?
>
> Thanks,
> Raymond.
>
>
> On Wed, Mar 17, 2021 at 6:14 AM Alexey Kukushkin <
> [hidden email]>
> wrote:
>
> > Pavel,
> >
> > My understanding might be wrong but I think the best practice (or even
> > strongly recommended way) to implement async methods in .NET is to
> execute
> > continuation on the caller's thread if ConfigureAwait(false) was not
> > specified. Pseudo-code might look like:
> >
> > async Task PutAsync(K k, V v)
> > {
> >     var continuationExecutor = configureAwait
> >         ? (SynchronizationContext.Current ?? TaskScheduler.Current)
> >         : null;
> >
> >     await <<async implementation>>
> >
> >     continuationExecutor.Post(continuation);
> > }
> >
> > I got this understanding from reading some blog
> > about SynchronizationContext lots of time ago. They were saying they
> > created SynchronizationContext specifically to allow posting
> continuations
> > to the caller's thread.
> >
> > The reason for that is to simplify the user's code to avoid routing in
> the
> > code. Suppose you have a UI (like WPF or WinForms) event handler that
> must
> > be processed on the U thread:
> >
> > async Task Button1_Click(EventArgs args)
> > {
> >     ignite.PutAsync(args.Key, args.Value);
> >     Button1.Disabled = true;
> > }
> >
> > Executing the "Button1.Disabled = true" on a ForkJoinPool pool would
> cause
> > a "Trying to modify UI on a non-UI thread" exception. But if you
> > capture SynchronizationContext.Current in PutAsync and then route
> > continuation to the captured context then the code would work.
> >
> > I think the users really expect the continuations to be executed on the
> > caller's thread.
> >
> > Sometimes you know that your continuation is really fast and safe and you
> > want to avoid switching threads to improve performance. In this case you
> > use ConfigureAwait(false) like
> >
> > ignite.PutAsync(args.Key, args.Value).ConfigureAwat(false);
> >
> > In this case the continuation executes on the Ignite thread without
> routing
> > to the caller's thread.
> >
> > вт, 16 мар. 2021 г. в 18:49, Pavel Tupitsyn <[hidden email]>:
> >
> > > Alexey,
> > >
> > > .NET thick API delegates to Java directly.
> > >
> > > When you do ICache.PutAsync():
> > > * Future is created on Java side, .listen() is called
> > > * TaskCompletionSource is created on .NET side, its Task is returned to
> > the
> > > user
> > > * Operation completes, Future listener is called on the Java side
> > > * Listener invokes JNI callback to .NET, where
> > > TaskCompletionSource.SetResult is called
> > >
> > > Therefore, .NET user code (in ContinueWith or after await) will be
> > executed
> > > on the Java
> > > thread that invokes the future listener.
> > >
> > > After the proposed fix, future listeners will be invoked on
> > > ForkJoinPool#commonPool (instead of striped pool).
> > > So .NET continuations will end up in commonPool as well, which solves
> the
> > > problem for .NET automatically, no changes required.
> > >
> > > Does that make sense?
> > >
> > > On Tue, Mar 16, 2021 at 1:52 PM Alexey Kukushkin <
> > > [hidden email]>
> > > wrote:
> > >
> > > > Hi Pavel,
> > > >
> > > > Extending Java async API with additional Executor parameters looks OK
> > to
> > > > me.
> > > >
> > > > It is not clear from the IEP how you are going to do that for .NET
> > async
> > > > API. My understanding is in .NET we do not add any Executors.
> Instead,
> > > the
> > > > Ignite Async API should use (SynchronizationContext.Current ??
> > > > TaskScheduler.Current) by default and it should have exciting
> behavior
> > > (use
> > > > Ignite striped pool) if ConfigureAwait(false) was specified for the
> > Task
> > > > result.
> > > >
> > > > Is my understanding correct?
> > > >
> > > >
> > > > пн, 15 мар. 2021 г. в 19:24, Pavel Tupitsyn <[hidden email]>:
> > > >
> > > > > Igniters,
> > > > >
> > > > > Please review the IEP [1] and let me know your thoughts.
> > > > >
> > > > > [1]
> > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/IGNITE/IEP-70%3A+Async+Continuation+Executor
> > > > >
> > > >
> > > >
> > > > --
> > > > Best regards,
> > > > Alexey
> > > >
> > >
> >
> >
> > --
> > Best regards,
> > Alexey
> >
>
>
> --
> <http://www.trimble.com/>
> Raymond Wilson
> Solution Architect, Civil Construction Software Systems (CCSS)
> 11 Birmingham Drive | Christchurch, New Zealand
> [hidden email]
>
> <
> https://worksos.trimble.com/?utm_source=Trimble&utm_medium=emailsign&utm_campaign=Launch
> >
>

--
Best regards,
Alexey

[Pavel Tupitsyn]

Alexey,

I'm not sure your understanding is entirely correct.
The proposal won't break or change anything for UI apps and other apps with
SynchronizationContext.
The code in Button1_Click works now and will work after the proposal:
continuations are routed to SynchronizationContext automatically when it is
present,
we don't have to worry about that in our code [1].

> Sometimes you know that your continuation is really fast and safe and you
> want to avoid switching threads to improve performance

This is a valid point, that's why the new behavior is configurable - safe
by default,
but can be overridden when you know what you are doing.

> In this case you use ConfigureAwait(false)

Not correct: ConfigureAwait() does nothing when there is no
SyncronizationContext:
in ASP.NET Core apps, in Console apps, basically everywhere except WPF,
WinForms, and Legacy ASP.NET.


[1] https://devblogs.microsoft.com/dotnet/configureawait-faq/

On Tue, Mar 16, 2021 at 10:54 PM Alexey Kukushkin <[hidden email]>
wrote:

> Raymond,
>
> The article you referenced
> <https://devblogs.microsoft.com/dotnet/configureawait-faq/> is great! It
> seems to me the article rather proved what I suggested: an async operation
> implementation:
>
>    - Uses the async operation thread (Ignite's thread) if ConfigureAwait is
>    false (by default it is true)
>    - Uses caller's current SynchornizationContext if it is specified
>    - Uses caller's TaskScheduler.Current if current
>    SynchornizationContext is null
>
> In the application code (outside framework callbacks) the
> SynchornizationContext.Current will be null and TaskScheduler.Current is
> the .NET's fork-join thread pool. Thus, normally the .NET thread pool would
> be used for continuations as Pavel suggested in the IEP.
>
> Executing Async operation in a context where
> SynchornizationContext.Current is not null means some framework explicitly
> configured the context and that means it is important to execute the
> continuation in that context (like UI or xUnit main thread)
>
> I agree that routing back to original context might result in waiting,
> which is very dangerous for an Ignite thread. We can create a worker thread
> to route continuation to original context.
>
>
> вт, 16 мар. 2021 г. в 21:40, Raymond Wilson <[hidden email]>:
>
> > There is a (long) discussion here (
> > https://devblogs.microsoft.com/dotnet/configureawait-faq/) regarding use
> > of
> > ConfigureAwait().
> >
> > Putting edge cases aside, there is a general use case for
> > .ConfigureAwait(true) in application UI contexts to ensure the
> continuation
> > joins a UI thread (for example), where failing to do so results in an
> > error.
> >
> > The other general use case relates more to library code where you are
> > typically running your tasks on the managed thread pool (assuming no
> custom
> > task schedulers etc). In this case the .library is agnostic about which
> > thread pool thread picks up the continuation, so forcing the continuation
> > to join to the original thread may be both a performance penalty from the
> > join overhead, but also may add latency waiting for that thread to become
> > available again.
> >
> > I don't have good insight into how the striped thread pool is used in
> > Ignite, but in highly concurrent environments letting those threads
> escape
> > into the calling client context seems like a bad idea in general.
> >
> > Stepping back a little, the Cache Async operations are good for when
> there
> > will be an IO operation incurred because the requested element is not
> > present in memory. If it is present in memory, then a Sync operation will
> > be more performant. Is it possible to do a two step operation like this
> > 'Cache.GetIfInMemory() ?? await Cache.GetAsync()' to allow the calling
> > context to optimise the calling model dynamically?
> >
> > Thanks,
> > Raymond.
> >
> >
> > On Wed, Mar 17, 2021 at 6:14 AM Alexey Kukushkin <
> > [hidden email]>
> > wrote:
> >
> > > Pavel,
> > >
> > > My understanding might be wrong but I think the best practice (or even
> > > strongly recommended way) to implement async methods in .NET is to
> > execute
> > > continuation on the caller's thread if ConfigureAwait(false) was not
> > > specified. Pseudo-code might look like:
> > >
> > > async Task PutAsync(K k, V v)
> > > {
> > >     var continuationExecutor = configureAwait
> > >         ? (SynchronizationContext.Current ?? TaskScheduler.Current)
> > >         : null;
> > >
> > >     await <<async implementation>>
> > >
> > >     continuationExecutor.Post(continuation);
> > > }
> > >
> > > I got this understanding from reading some blog
> > > about SynchronizationContext lots of time ago. They were saying they
> > > created SynchronizationContext specifically to allow posting
> > continuations
> > > to the caller's thread.
> > >
> > > The reason for that is to simplify the user's code to avoid routing in
> > the
> > > code. Suppose you have a UI (like WPF or WinForms) event handler that
> > must
> > > be processed on the U thread:
> > >
> > > async Task Button1_Click(EventArgs args)
> > > {
> > >     ignite.PutAsync(args.Key, args.Value);
> > >     Button1.Disabled = true;
> > > }
> > >
> > > Executing the "Button1.Disabled = true" on a ForkJoinPool pool would
> > cause
> > > a "Trying to modify UI on a non-UI thread" exception. But if you
> > > capture SynchronizationContext.Current in PutAsync and then route
> > > continuation to the captured context then the code would work.
> > >
> > > I think the users really expect the continuations to be executed on the
> > > caller's thread.
> > >
> > > Sometimes you know that your continuation is really fast and safe and
> you
> > > want to avoid switching threads to improve performance. In this case
> you
> > > use ConfigureAwait(false) like
> > >
> > > ignite.PutAsync(args.Key, args.Value).ConfigureAwat(false);
> > >
> > > In this case the continuation executes on the Ignite thread without
> > routing
> > > to the caller's thread.
> > >
> > > вт, 16 мар. 2021 г. в 18:49, Pavel Tupitsyn <[hidden email]>:
> > >
> > > > Alexey,
> > > >
> > > > .NET thick API delegates to Java directly.
> > > >
> > > > When you do ICache.PutAsync():
> > > > * Future is created on Java side, .listen() is called
> > > > * TaskCompletionSource is created on .NET side, its Task is returned
> to
> > > the
> > > > user
> > > > * Operation completes, Future listener is called on the Java side
> > > > * Listener invokes JNI callback to .NET, where
> > > > TaskCompletionSource.SetResult is called
> > > >
> > > > Therefore, .NET user code (in ContinueWith or after await) will be
> > > executed
> > > > on the Java
> > > > thread that invokes the future listener.
> > > >
> > > > After the proposed fix, future listeners will be invoked on
> > > > ForkJoinPool#commonPool (instead of striped pool).
> > > > So .NET continuations will end up in commonPool as well, which solves
> > the
> > > > problem for .NET automatically, no changes required.
> > > >
> > > > Does that make sense?
> > > >
> > > > On Tue, Mar 16, 2021 at 1:52 PM Alexey Kukushkin <
> > > > [hidden email]>
> > > > wrote:
> > > >
> > > > > Hi Pavel,
> > > > >
> > > > > Extending Java async API with additional Executor parameters looks
> OK
> > > to
> > > > > me.
> > > > >
> > > > > It is not clear from the IEP how you are going to do that for .NET
> > > async
> > > > > API. My understanding is in .NET we do not add any Executors.
> > Instead,
> > > > the
> > > > > Ignite Async API should use (SynchronizationContext.Current ??
> > > > > TaskScheduler.Current) by default and it should have exciting
> > behavior
> > > > (use
> > > > > Ignite striped pool) if ConfigureAwait(false) was specified for the
> > > Task
> > > > > result.
> > > > >
> > > > > Is my understanding correct?
> > > > >
> > > > >
> > > > > пн, 15 мар. 2021 г. в 19:24, Pavel Tupitsyn <[hidden email]
> >:
> > > > >
> > > > > > Igniters,
> > > > > >
> > > > > > Please review the IEP [1] and let me know your thoughts.
> > > > > >
> > > > > > [1]
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/IGNITE/IEP-70%3A+Async+Continuation+Executor
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Best regards,
> > > > > Alexey
> > > > >
> > > >
> > >
> > >
> > > --
> > > Best regards,
> > > Alexey
> > >
> >
> >
> > --
> > <http://www.trimble.com/>
> > Raymond Wilson
> > Solution Architect, Civil Construction Software Systems (CCSS)
> > 11 Birmingham Drive | Christchurch, New Zealand
> > [hidden email]
> >
> > <
> >
> https://worksos.trimble.com/?utm_source=Trimble&utm_medium=emailsign&utm_campaign=Launch
> > >
> >
>
>
> --
> Best regards,
> Alexey
>

[Pavel Tupitsyn]

Raymond,

> If it is present in memory, then a Sync operation will
> be more performant. Is it possible to do a two step operation like this
> 'Cache.GetIfInMemory() ?? await Cache.GetAsync()'

Ignite already does that - if the key is present locally (primary, backup,
near cache, platform cache),
the operation will be performed synchronously and you'll get a completed
Task back;
there won't be any thread jumps.

We'll make sure this does not change with the IEP - I'll add tests.

Having said that, you can do a two-step op:
cache.TryLocalPeek(1, out var v) ? v : await cache.GetAsync(1)

This performs better for local entries, because we avoid
async method overhead, and worse for non-local entries, because we do two
calls.

On Tue, Mar 16, 2021 at 10:58 PM Pavel Tupitsyn <[hidden email]>
wrote:

> Alexey,
>
> I'm not sure your understanding is entirely correct.
> The proposal won't break or change anything for UI apps and other apps
> with SynchronizationContext.
> The code in Button1_Click works now and will work after the proposal:
> continuations are routed to SynchronizationContext automatically when it
> is present,
> we don't have to worry about that in our code [1].
>
> > Sometimes you know that your continuation is really fast and safe and you
> > want to avoid switching threads to improve performance
>
> This is a valid point, that's why the new behavior is configurable - safe
> by default,
> but can be overridden when you know what you are doing.
>
> > In this case you use ConfigureAwait(false)
>
> Not correct: ConfigureAwait() does nothing when there is no
> SyncronizationContext:
> in ASP.NET Core apps, in Console apps, basically everywhere except WPF,
> WinForms, and Legacy ASP.NET.
>
>
> [1] https://devblogs.microsoft.com/dotnet/configureawait-faq/
>
> On Tue, Mar 16, 2021 at 10:54 PM Alexey Kukushkin <
> [hidden email]> wrote:
>
>> Raymond,
>>
>> The article you referenced
>> <https://devblogs.microsoft.com/dotnet/configureawait-faq/> is great! It
>> seems to me the article rather proved what I suggested: an async operation
>> implementation:
>>
>>    - Uses the async operation thread (Ignite's thread) if ConfigureAwait
>> is
>>    false (by default it is true)
>>    - Uses caller's current SynchornizationContext if it is specified
>>    - Uses caller's TaskScheduler.Current if current
>>    SynchornizationContext is null
>>
>> In the application code (outside framework callbacks) the
>> SynchornizationContext.Current will be null and TaskScheduler.Current is
>> the .NET's fork-join thread pool. Thus, normally the .NET thread pool
>> would
>> be used for continuations as Pavel suggested in the IEP.
>>
>> Executing Async operation in a context where
>> SynchornizationContext.Current is not null means some framework explicitly
>> configured the context and that means it is important to execute the
>> continuation in that context (like UI or xUnit main thread)
>>
>> I agree that routing back to original context might result in waiting,
>> which is very dangerous for an Ignite thread. We can create a worker
>> thread
>> to route continuation to original context.
>>
>>
>> вт, 16 мар. 2021 г. в 21:40, Raymond Wilson <[hidden email]>:
>>
>> > There is a (long) discussion here (
>> > https://devblogs.microsoft.com/dotnet/configureawait-faq/) regarding
>> use
>> > of
>> > ConfigureAwait().
>> >
>> > Putting edge cases aside, there is a general use case for
>> > .ConfigureAwait(true) in application UI contexts to ensure the
>> continuation
>> > joins a UI thread (for example), where failing to do so results in an
>> > error.
>> >
>> > The other general use case relates more to library code where you are
>> > typically running your tasks on the managed thread pool (assuming no
>> custom
>> > task schedulers etc). In this case the .library is agnostic about which
>> > thread pool thread picks up the continuation, so forcing the
>> continuation
>> > to join to the original thread may be both a performance penalty from
>> the
>> > join overhead, but also may add latency waiting for that thread to
>> become
>> > available again.
>> >
>> > I don't have good insight into how the striped thread pool is used in
>> > Ignite, but in highly concurrent environments letting those threads
>> escape
>> > into the calling client context seems like a bad idea in general.
>> >
>> > Stepping back a little, the Cache Async operations are good for when
>> there
>> > will be an IO operation incurred because the requested element is not
>> > present in memory. If it is present in memory, then a Sync operation
>> will
>> > be more performant. Is it possible to do a two step operation like this
>> > 'Cache.GetIfInMemory() ?? await Cache.GetAsync()' to allow the calling
>> > context to optimise the calling model dynamically?
>> >
>> > Thanks,
>> > Raymond.
>> >
>> >
>> > On Wed, Mar 17, 2021 at 6:14 AM Alexey Kukushkin <
>> > [hidden email]>
>> > wrote:
>> >
>> > > Pavel,
>> > >
>> > > My understanding might be wrong but I think the best practice (or even
>> > > strongly recommended way) to implement async methods in .NET is to
>> > execute
>> > > continuation on the caller's thread if ConfigureAwait(false) was not
>> > > specified. Pseudo-code might look like:
>> > >
>> > > async Task PutAsync(K k, V v)
>> > > {
>> > >     var continuationExecutor = configureAwait
>> > >         ? (SynchronizationContext.Current ?? TaskScheduler.Current)
>> > >         : null;
>> > >
>> > >     await <<async implementation>>
>> > >
>> > >     continuationExecutor.Post(continuation);
>> > > }
>> > >
>> > > I got this understanding from reading some blog
>> > > about SynchronizationContext lots of time ago. They were saying they
>> > > created SynchronizationContext specifically to allow posting
>> > continuations
>> > > to the caller's thread.
>> > >
>> > > The reason for that is to simplify the user's code to avoid routing in
>> > the
>> > > code. Suppose you have a UI (like WPF or WinForms) event handler that
>> > must
>> > > be processed on the U thread:
>> > >
>> > > async Task Button1_Click(EventArgs args)
>> > > {
>> > >     ignite.PutAsync(args.Key, args.Value);
>> > >     Button1.Disabled = true;
>> > > }
>> > >
>> > > Executing the "Button1.Disabled = true" on a ForkJoinPool pool would
>> > cause
>> > > a "Trying to modify UI on a non-UI thread" exception. But if you
>> > > capture SynchronizationContext.Current in PutAsync and then route
>> > > continuation to the captured context then the code would work.
>> > >
>> > > I think the users really expect the continuations to be executed on
>> the
>> > > caller's thread.
>> > >
>> > > Sometimes you know that your continuation is really fast and safe and
>> you
>> > > want to avoid switching threads to improve performance. In this case
>> you
>> > > use ConfigureAwait(false) like
>> > >
>> > > ignite.PutAsync(args.Key, args.Value).ConfigureAwat(false);
>> > >
>> > > In this case the continuation executes on the Ignite thread without
>> > routing
>> > > to the caller's thread.
>> > >
>> > > вт, 16 мар. 2021 г. в 18:49, Pavel Tupitsyn <[hidden email]>:
>> > >
>> > > > Alexey,
>> > > >
>> > > > .NET thick API delegates to Java directly.
>> > > >
>> > > > When you do ICache.PutAsync():
>> > > > * Future is created on Java side, .listen() is called
>> > > > * TaskCompletionSource is created on .NET side, its Task is
>> returned to
>> > > the
>> > > > user
>> > > > * Operation completes, Future listener is called on the Java side
>> > > > * Listener invokes JNI callback to .NET, where
>> > > > TaskCompletionSource.SetResult is called
>> > > >
>> > > > Therefore, .NET user code (in ContinueWith or after await) will be
>> > > executed
>> > > > on the Java
>> > > > thread that invokes the future listener.
>> > > >
>> > > > After the proposed fix, future listeners will be invoked on
>> > > > ForkJoinPool#commonPool (instead of striped pool).
>> > > > So .NET continuations will end up in commonPool as well, which
>> solves
>> > the
>> > > > problem for .NET automatically, no changes required.
>> > > >
>> > > > Does that make sense?
>> > > >
>> > > > On Tue, Mar 16, 2021 at 1:52 PM Alexey Kukushkin <
>> > > > [hidden email]>
>> > > > wrote:
>> > > >
>> > > > > Hi Pavel,
>> > > > >
>> > > > > Extending Java async API with additional Executor parameters
>> looks OK
>> > > to
>> > > > > me.
>> > > > >
>> > > > > It is not clear from the IEP how you are going to do that for .NET
>> > > async
>> > > > > API. My understanding is in .NET we do not add any Executors.
>> > Instead,
>> > > > the
>> > > > > Ignite Async API should use (SynchronizationContext.Current ??
>> > > > > TaskScheduler.Current) by default and it should have exciting
>> > behavior
>> > > > (use
>> > > > > Ignite striped pool) if ConfigureAwait(false) was specified for
>> the
>> > > Task
>> > > > > result.
>> > > > >
>> > > > > Is my understanding correct?
>> > > > >
>> > > > >
>> > > > > пн, 15 мар. 2021 г. в 19:24, Pavel Tupitsyn <[hidden email]
>> >:
>> > > > >
>> > > > > > Igniters,
>> > > > > >
>> > > > > > Please review the IEP [1] and let me know your thoughts.
>> > > > > >
>> > > > > > [1]
>> > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>> https://cwiki.apache.org/confluence/display/IGNITE/IEP-70%3A+Async+Continuation+Executor
>> > > > > >
>> > > > >
>> > > > >
>> > > > > --
>> > > > > Best regards,
>> > > > > Alexey
>> > > > >
>> > > >
>> > >
>> > >
>> > > --
>> > > Best regards,
>> > > Alexey
>> > >
>> >
>> >
>> > --
>> > <http://www.trimble.com/>
>> > Raymond Wilson
>> > Solution Architect, Civil Construction Software Systems (CCSS)
>> > 11 Birmingham Drive | Christchurch, New Zealand
>> > [hidden email]
>> >
>> > <
>> >
>> https://worksos.trimble.com/?utm_source=Trimble&utm_medium=emailsign&utm_campaign=Launch
>> > >
>> >
>>
>>
>> --
>> Best regards,
>> Alexey
>>
>

[Alexey Kukushkin]

Pavel,

So what you are saying is submitting a continuation to .NET Thread Pool
already respects current  SynchronizationContext and ConfigureAwait. In
this case the IEP looks complete (for some reason I thought that we should
take care about the SynchronizationContext inside the Ignite.NET
implementation).

--
Best regards,
Alexey

[Denis Garus]

Hello!

As I understood, we don't try to solve the problem mentioned in the IEP:
there is unexpected behavior,
users should carefully read the docs to know about this, and so on.
A thread that executes an incorrect listener hung in any case,
and the suggested solution is to change the pool which owns this thread.
Did you consider an option to execute user-defined listeners with a timeout?
I think that implicit using java pools to run a code that can be hung is a
questionable solution.

вт, 16 мар. 2021 г. в 23:30, Alexey Kukushkin <[hidden email]>:

> Pavel,
>
> So what you are saying is submitting a continuation to .NET Thread Pool
> already respects current  SynchronizationContext and ConfigureAwait. In
> this case the IEP looks complete (for some reason I thought that we should
> take care about the SynchronizationContext inside the Ignite.NET
> implementation).
>
> --
> Best regards,
> Alexey
>

[Pavel Tupitsyn]

Denis,

> we don't try to solve the problem mentioned in the IEP

The problem: user code executes on striped pool (which causes issues)
The solution: don't execute user code on striped pool

I believe this solves the problem and removes any and all restrictions
for async listeners/continuations. Am I missing something else?

On Wed, Mar 17, 2021 at 10:16 AM Denis Garus <[hidden email]> wrote:

> Hello!
>
> As I understood, we don't try to solve the problem mentioned in the IEP:
> there is unexpected behavior,
> users should carefully read the docs to know about this, and so on.
> A thread that executes an incorrect listener hung in any case,
> and the suggested solution is to change the pool which owns this thread.
> Did you consider an option to execute user-defined listeners with a
> timeout?
> I think that implicit using java pools to run a code that can be hung is a
> questionable solution.
>
> вт, 16 мар. 2021 г. в 23:30, Alexey Kukushkin <[hidden email]>:
>
> > Pavel,
> >
> > So what you are saying is submitting a continuation to .NET Thread Pool
> > already respects current  SynchronizationContext and ConfigureAwait. In
> > this case the IEP looks complete (for some reason I thought that we
> should
> > take care about the SynchronizationContext inside the Ignite.NET
> > implementation).
> >
> > --
> > Best regards,
> > Alexey
> >
>

[Alexey Kukushkin]

My initial confusion was due to I did not know the
TaskCompletionSource.SetResult() internally handles
SynchronizationContext captured before the async operation. I thought we
would have to do it in Ignite. Now I know that and have no problems with
the IEP.

ср, 17 мар. 2021 г. в 11:18, Pavel Tupitsyn <[hidden email]>:

> Denis,
>
> > we don't try to solve the problem mentioned in the IEP
>
> The problem: user code executes on striped pool (which causes issues)
> The solution: don't execute user code on striped pool
>
> I believe this solves the problem and removes any and all restrictions
> for async listeners/continuations. Am I missing something else?
>
> On Wed, Mar 17, 2021 at 10:16 AM Denis Garus <[hidden email]> wrote:
>
> > Hello!
> >
> > As I understood, we don't try to solve the problem mentioned in the IEP:
> > there is unexpected behavior,
> > users should carefully read the docs to know about this, and so on.
> > A thread that executes an incorrect listener hung in any case,
> > and the suggested solution is to change the pool which owns this thread.
> > Did you consider an option to execute user-defined listeners with a
> > timeout?
> > I think that implicit using java pools to run a code that can be hung is
> a
> > questionable solution.
> >
> > вт, 16 мар. 2021 г. в 23:30, Alexey Kukushkin <[hidden email]
> >:
> >
> > > Pavel,
> > >
> > > So what you are saying is submitting a continuation to .NET Thread Pool
> > > already respects current  SynchronizationContext and ConfigureAwait. In
> > > this case the IEP looks complete (for some reason I thought that we
> > should
> > > take care about the SynchronizationContext inside the Ignite.NET
> > > implementation).
> > >
> > > --
> > > Best regards,
> > > Alexey
> > >
> >
>

--
Best regards,
Alexey

[Denis Garus]

Pavel, thank you for your answer.

Sorry, the previous version of IEP motivation highlighted what could be
wrong with user listeners,
so I thought that is the main problem.

I'm wondering why you suggest using ForkJoinPool.commonPool as the default
pool to run listeners?

ср, 17 мар. 2021 г. в 11:51, Alexey Kukushkin <[hidden email]>:

> My initial confusion was due to I did not know the
> TaskCompletionSource.SetResult() internally handles
> SynchronizationContext captured before the async operation. I thought we
> would have to do it in Ignite. Now I know that and have no problems with
> the IEP.
>
> ср, 17 мар. 2021 г. в 11:18, Pavel Tupitsyn <[hidden email]>:
>
> > Denis,
> >
> > > we don't try to solve the problem mentioned in the IEP
> >
> > The problem: user code executes on striped pool (which causes issues)
> > The solution: don't execute user code on striped pool
> >
> > I believe this solves the problem and removes any and all restrictions
> > for async listeners/continuations. Am I missing something else?
> >
> > On Wed, Mar 17, 2021 at 10:16 AM Denis Garus <[hidden email]>
> wrote:
> >
> > > Hello!
> > >
> > > As I understood, we don't try to solve the problem mentioned in the
> IEP:
> > > there is unexpected behavior,
> > > users should carefully read the docs to know about this, and so on.
> > > A thread that executes an incorrect listener hung in any case,
> > > and the suggested solution is to change the pool which owns this
> thread.
> > > Did you consider an option to execute user-defined listeners with a
> > > timeout?
> > > I think that implicit using java pools to run a code that can be hung
> is
> > a
> > > questionable solution.
> > >
> > > вт, 16 мар. 2021 г. в 23:30, Alexey Kukushkin <
> [hidden email]
> > >:
> > >
> > > > Pavel,
> > > >
> > > > So what you are saying is submitting a continuation to .NET Thread
> Pool
> > > > already respects current  SynchronizationContext and ConfigureAwait.
> In
> > > > this case the IEP looks complete (for some reason I thought that we
> > > should
> > > > take care about the SynchronizationContext inside the Ignite.NET
> > > > implementation).
> > > >
> > > > --
> > > > Best regards,
> > > > Alexey
> > > >
> > >
> >
>
>
> --
> Best regards,
> Alexey
>

[Pavel Tupitsyn]

Denis,

Yes, I've updated the Motivation section, it really was a bit vague - thank
you.

> why you suggest using ForkJoinPool.commonPool as the default pool to run
listeners

- It is recommended to be used by default [1]
- There are no alternatives?

[1]
https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ForkJoinPool.html

On Wed, Mar 17, 2021 at 1:02 PM Denis Garus <[hidden email]> wrote:

> Pavel, thank you for your answer.
>
> Sorry, the previous version of IEP motivation highlighted what could be
> wrong with user listeners,
> so I thought that is the main problem.
>
> I'm wondering why you suggest using ForkJoinPool.commonPool as the default
> pool to run listeners?
>
> ср, 17 мар. 2021 г. в 11:51, Alexey Kukushkin <[hidden email]>:
>
> > My initial confusion was due to I did not know the
> > TaskCompletionSource.SetResult() internally handles
> > SynchronizationContext captured before the async operation. I thought we
> > would have to do it in Ignite. Now I know that and have no problems with
> > the IEP.
> >
> > ср, 17 мар. 2021 г. в 11:18, Pavel Tupitsyn <[hidden email]>:
> >
> > > Denis,
> > >
> > > > we don't try to solve the problem mentioned in the IEP
> > >
> > > The problem: user code executes on striped pool (which causes issues)
> > > The solution: don't execute user code on striped pool
> > >
> > > I believe this solves the problem and removes any and all restrictions
> > > for async listeners/continuations. Am I missing something else?
> > >
> > > On Wed, Mar 17, 2021 at 10:16 AM Denis Garus <[hidden email]>
> > wrote:
> > >
> > > > Hello!
> > > >
> > > > As I understood, we don't try to solve the problem mentioned in the
> > IEP:
> > > > there is unexpected behavior,
> > > > users should carefully read the docs to know about this, and so on.
> > > > A thread that executes an incorrect listener hung in any case,
> > > > and the suggested solution is to change the pool which owns this
> > thread.
> > > > Did you consider an option to execute user-defined listeners with a
> > > > timeout?
> > > > I think that implicit using java pools to run a code that can be hung
> > is
> > > a
> > > > questionable solution.
> > > >
> > > > вт, 16 мар. 2021 г. в 23:30, Alexey Kukushkin <
> > [hidden email]
> > > >:
> > > >
> > > > > Pavel,
> > > > >
> > > > > So what you are saying is submitting a continuation to .NET Thread
> > Pool
> > > > > already respects current  SynchronizationContext and
> ConfigureAwait.
> > In
> > > > > this case the IEP looks complete (for some reason I thought that we
> > > > should
> > > > > take care about the SynchronizationContext inside the Ignite.NET
> > > > > implementation).
> > > > >
> > > > > --
> > > > > Best regards,
> > > > > Alexey
> > > > >
> > > >
> > >
> >
> >
> > --
> > Best regards,
> > Alexey
> >
>

[slava.koptilin]

Hello Pavel,

I took a look at your IEP and pool request. I have the following concerns.
First of all, this change breaks the contract of IgniteFuture#listen(lsnr)

    /**
     * Registers listener closure to be asynchronously notified whenever
future completes.
     * Closure will be processed in thread that completes this future or
(if future already
     * completed) immediately in current thread.
     *
     * @param lsnr Listener closure to register. Cannot be {@code null}.
     */
    public void listen(IgniteInClosure<? super IgniteFuture<V>> lsnr);

    In your pull request, the listener is always called from a specified
thread pool (which is fork-join by default)
    even though the future is already completed at the moment the listen
method is called.
    In my opinion, this can lead to significant overhead - submission
requires acquiring a lock and notifying a pool thread.

    It seems to me, that we should not change the current behavior.
However, thread pool executor can be added as an optional parameter of
listen() method as follows:

        public void listen(IgniteInClosure<? super IgniteFuture<V>> lsnr,
Executor exec);

Thanks,
S.

пн, 15 мар. 2021 г. в 19:24, Pavel Tupitsyn <[hidden email]>:

> Igniters,
>
> Please review the IEP [1] and let me know your thoughts.
>
> [1]
>
> https://cwiki.apache.org/confluence/display/IGNITE/IEP-70%3A+Async+Continuation+Executor
>

[Denis Garus]

Pavel, I got it.
Thank you.

ср, 17 мар. 2021 г. в 13:11, Pavel Tupitsyn <[hidden email]>:

> Denis,
>
> Yes, I've updated the Motivation section, it really was a bit vague - thank
> you.
>
> > why you suggest using ForkJoinPool.commonPool as the default pool to run
> listeners
>
> - It is recommended to be used by default [1]
> - There are no alternatives?
>
> [1]
>
> https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ForkJoinPool.html
>
> On Wed, Mar 17, 2021 at 1:02 PM Denis Garus <[hidden email]> wrote:
>
> > Pavel, thank you for your answer.
> >
> > Sorry, the previous version of IEP motivation highlighted what could be
> > wrong with user listeners,
> > so I thought that is the main problem.
> >
> > I'm wondering why you suggest using ForkJoinPool.commonPool as the
> default
> > pool to run listeners?
> >
> > ср, 17 мар. 2021 г. в 11:51, Alexey Kukushkin <[hidden email]
> >:
> >
> > > My initial confusion was due to I did not know the
> > > TaskCompletionSource.SetResult() internally handles
> > > SynchronizationContext captured before the async operation. I thought
> we
> > > would have to do it in Ignite. Now I know that and have no problems
> with
> > > the IEP.
> > >
> > > ср, 17 мар. 2021 г. в 11:18, Pavel Tupitsyn <[hidden email]>:
> > >
> > > > Denis,
> > > >
> > > > > we don't try to solve the problem mentioned in the IEP
> > > >
> > > > The problem: user code executes on striped pool (which causes issues)
> > > > The solution: don't execute user code on striped pool
> > > >
> > > > I believe this solves the problem and removes any and all
> restrictions
> > > > for async listeners/continuations. Am I missing something else?
> > > >
> > > > On Wed, Mar 17, 2021 at 10:16 AM Denis Garus <[hidden email]>
> > > wrote:
> > > >
> > > > > Hello!
> > > > >
> > > > > As I understood, we don't try to solve the problem mentioned in the
> > > IEP:
> > > > > there is unexpected behavior,
> > > > > users should carefully read the docs to know about this, and so on.
> > > > > A thread that executes an incorrect listener hung in any case,
> > > > > and the suggested solution is to change the pool which owns this
> > > thread.
> > > > > Did you consider an option to execute user-defined listeners with a
> > > > > timeout?
> > > > > I think that implicit using java pools to run a code that can be
> hung
> > > is
> > > > a
> > > > > questionable solution.
> > > > >
> > > > > вт, 16 мар. 2021 г. в 23:30, Alexey Kukushkin <
> > > [hidden email]
> > > > >:
> > > > >
> > > > > > Pavel,
> > > > > >
> > > > > > So what you are saying is submitting a continuation to .NET
> Thread
> > > Pool
> > > > > > already respects current  SynchronizationContext and
> > ConfigureAwait.
> > > In
> > > > > > this case the IEP looks complete (for some reason I thought that
> we
> > > > > should
> > > > > > take care about the SynchronizationContext inside the Ignite.NET
> > > > > > implementation).
> > > > > >
> > > > > > --
> > > > > > Best regards,
> > > > > > Alexey
> > > > > >
> > > > >
> > > >
> > >
> > >
> > > --
> > > Best regards,
> > > Alexey
> > >
> >
>

[slava.koptilin]

Well, the specified method already exists :)

    /**
     * Registers listener closure to be asynchronously notified whenever
future completes.
     * Closure will be processed in specified executor.
     *
     * @param lsnr Listener closure to register. Cannot be {@code null}.
     * @param exec Executor to run listener. Cannot be {@code null}.
     */
    public void listenAsync(IgniteInClosure<? super IgniteFuture<V>> lsnr,
Executor exec);

Thanks,
S.

ср, 17 мар. 2021 г. в 15:25, Вячеслав Коптилин <[hidden email]>:

> Hello Pavel,
>
> I took a look at your IEP and pool request. I have the following concerns.
> First of all, this change breaks the contract of IgniteFuture#listen(lsnr)
>
>     /**
>      * Registers listener closure to be asynchronously notified whenever
> future completes.
>      * Closure will be processed in thread that completes this future or
> (if future already
>      * completed) immediately in current thread.
>      *
>      * @param lsnr Listener closure to register. Cannot be {@code null}.
>      */
>     public void listen(IgniteInClosure<? super IgniteFuture<V>> lsnr);
>
>     In your pull request, the listener is always called from a specified
> thread pool (which is fork-join by default)
>     even though the future is already completed at the moment the listen
> method is called.
>     In my opinion, this can lead to significant overhead - submission
> requires acquiring a lock and notifying a pool thread.
>
>     It seems to me, that we should not change the current behavior.
> However, thread pool executor can be added as an optional parameter of
> listen() method as follows:
>
>         public void listen(IgniteInClosure<? super IgniteFuture<V>> lsnr,
> Executor exec);
>
> Thanks,
> S.
>
> пн, 15 мар. 2021 г. в 19:24, Pavel Tupitsyn <[hidden email]>:
>
>> Igniters,
>>
>> Please review the IEP [1] and let me know your thoughts.
>>
>> [1]
>>
>> https://cwiki.apache.org/confluence/display/IGNITE/IEP-70%3A+Async+Continuation+Executor
>>
>

[Pavel Tupitsyn]

Slava,

Your suggestion is to keep things as is and discard the IEP, right?

>  this can lead to significant overhead
Yes, there is some overhead, but the cost of accidentally starving the
striped pool is worse,
not to mention the deadlocks.

I believe that we should favor correctness over performance in any case.


On Wed, Mar 17, 2021 at 3:34 PM Вячеслав Коптилин <[hidden email]>
wrote:

> Well, the specified method already exists :)
>
>     /**
>      * Registers listener closure to be asynchronously notified whenever
> future completes.
>      * Closure will be processed in specified executor.
>      *
>      * @param lsnr Listener closure to register. Cannot be {@code null}.
>      * @param exec Executor to run listener. Cannot be {@code null}.
>      */
>     public void listenAsync(IgniteInClosure<? super IgniteFuture<V>> lsnr,
> Executor exec);
>
> Thanks,
> S.
>
> ср, 17 мар. 2021 г. в 15:25, Вячеслав Коптилин <[hidden email]>:
>
> > Hello Pavel,
> >
> > I took a look at your IEP and pool request. I have the following
> concerns.
> > First of all, this change breaks the contract of
> IgniteFuture#listen(lsnr)
> >
> >     /**
> >      * Registers listener closure to be asynchronously notified whenever
> > future completes.
> >      * Closure will be processed in thread that completes this future or
> > (if future already
> >      * completed) immediately in current thread.
> >      *
> >      * @param lsnr Listener closure to register. Cannot be {@code null}.
> >      */
> >     public void listen(IgniteInClosure<? super IgniteFuture<V>> lsnr);
> >
> >     In your pull request, the listener is always called from a specified
> > thread pool (which is fork-join by default)
> >     even though the future is already completed at the moment the listen
> > method is called.
> >     In my opinion, this can lead to significant overhead - submission
> > requires acquiring a lock and notifying a pool thread.
> >
> >     It seems to me, that we should not change the current behavior.
> > However, thread pool executor can be added as an optional parameter of
> > listen() method as follows:
> >
> >         public void listen(IgniteInClosure<? super IgniteFuture<V>> lsnr,
> > Executor exec);
> >
> > Thanks,
> > S.
> >
> > пн, 15 мар. 2021 г. в 19:24, Pavel Tupitsyn <[hidden email]>:
> >
> >> Igniters,
> >>
> >> Please review the IEP [1] and let me know your thoughts.
> >>
> >> [1]
> >>
> >>
> https://cwiki.apache.org/confluence/display/IGNITE/IEP-70%3A+Async+Continuation+Executor
> >>
> >
>

[slava.koptilin]

Hi Pavel,

Well, I think that the user should use the right API instead of introducing
uncontested overhead for everyone.
For instance, the code that is provided by IEP can changed as follows:

IgniteFuture fut = cache.putAsync(1, 1);
fut.listenAync(f -> {
    // Executes on Striped pool and deadlocks.
    cache.replace(1, 2);
}, ForkJoinPool.commonPool());

Of course, it does not mean that this fact should not be properly
documented.
Perhaps, I am missing something.

Thanks,
S.

ср, 17 мар. 2021 г. в 16:01, Pavel Tupitsyn <[hidden email]>:

> Slava,
>
> Your suggestion is to keep things as is and discard the IEP, right?
>
> >  this can lead to significant overhead
> Yes, there is some overhead, but the cost of accidentally starving the
> striped pool is worse,
> not to mention the deadlocks.
>
> I believe that we should favor correctness over performance in any case.
>
>
> On Wed, Mar 17, 2021 at 3:34 PM Вячеслав Коптилин <
> [hidden email]>
> wrote:
>
> > Well, the specified method already exists :)
> >
> >     /**
> >      * Registers listener closure to be asynchronously notified whenever
> > future completes.
> >      * Closure will be processed in specified executor.
> >      *
> >      * @param lsnr Listener closure to register. Cannot be {@code null}.
> >      * @param exec Executor to run listener. Cannot be {@code null}.
> >      */
> >     public void listenAsync(IgniteInClosure<? super IgniteFuture<V>>
> lsnr,
> > Executor exec);
> >
> > Thanks,
> > S.
> >
> > ср, 17 мар. 2021 г. в 15:25, Вячеслав Коптилин <[hidden email]
> >:
> >
> > > Hello Pavel,
> > >
> > > I took a look at your IEP and pool request. I have the following
> > concerns.
> > > First of all, this change breaks the contract of
> > IgniteFuture#listen(lsnr)
> > >
> > >     /**
> > >      * Registers listener closure to be asynchronously notified
> whenever
> > > future completes.
> > >      * Closure will be processed in thread that completes this future
> or
> > > (if future already
> > >      * completed) immediately in current thread.
> > >      *
> > >      * @param lsnr Listener closure to register. Cannot be {@code
> null}.
> > >      */
> > >     public void listen(IgniteInClosure<? super IgniteFuture<V>> lsnr);
> > >
> > >     In your pull request, the listener is always called from a
> specified
> > > thread pool (which is fork-join by default)
> > >     even though the future is already completed at the moment the
> listen
> > > method is called.
> > >     In my opinion, this can lead to significant overhead - submission
> > > requires acquiring a lock and notifying a pool thread.
> > >
> > >     It seems to me, that we should not change the current behavior.
> > > However, thread pool executor can be added as an optional parameter of
> > > listen() method as follows:
> > >
> > >         public void listen(IgniteInClosure<? super IgniteFuture<V>>
> lsnr,
> > > Executor exec);
> > >
> > > Thanks,
> > > S.
> > >
> > > пн, 15 мар. 2021 г. в 19:24, Pavel Tupitsyn <[hidden email]>:
> > >
> > >> Igniters,
> > >>
> > >> Please review the IEP [1] and let me know your thoughts.
> > >>
> > >> [1]
> > >>
> > >>
> >
> https://cwiki.apache.org/confluence/display/IGNITE/IEP-70%3A+Async+Continuation+Executor
> > >>
> > >
> >
>