You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@reef.apache.org by Markus Weimer <ma...@weimo.de> on 2015/11/19 17:55:48 UTC
Does ITask need to be IDisposable?
Hi,
currently, `ITask` is `IDisposable`, all our tasks need to implement a
`Dispose()` method. I'm not convinced this needs to be the case:
First of all, it clutters the code of many REEF Tasks, which is
especially annoying in our Examples. Secondly, we raise an event after a
Task finishes, which makes `Dispose()` redundant. Also, the `Call()`
method can just do whatever `Dispose()` would do at its end.
Hence, I propose to `[Obsolete]` `ITask.Dispose()` in 0.14 and to plan
for its removal in 0.15.
Thoughts?
Markus
Re: Does ITask need to be IDisposable?
Posted by Markus Weimer <ma...@weimo.de>.
On 2015-11-19 15:34, Julia Wang (QIUHE) wrote:
> We get Task object by injecting usually and we need to keep the task
> reference as ITask so that it can call ITask methods. On another
> side, to put task inside using, it must be an IDisposable. That is
> the reason the following syntax is used.
There is nothing below this. Could it be that Outlook put an image or
such here? Apache mailing lists are text-only and what you see in
Outlook before hitting the send button is not what we see :-(
Markus
RE: Does ITask need to be IDisposable?
Posted by "Julia Wang (QIUHE)" <Qi...@microsoft.com>.
We get Task object by injecting usually and we need to keep the task reference as ITask so that it can call ITask methods. On another side, to put task inside using, it must be an IDisposable. That is the reason the following syntax is used.
If ITask inherits from IDisposable, then we can put ITask inside using directly.
-----Original Message-----
From: Boris Shulman [mailto:shulmanb@gmail.com]
Sent: Thursday, November 19, 2015 3:25 PM
To: dev@reef.incubator.apache.org
Subject: Re: Does ITask need to be IDisposable?
Can we agree on doing:
var task = CreateTask();
using(task as IDisposable) // null is handled by the compiler transformation.
{
task.Foo();
}
?
On Thu, Nov 19, 2015 at 1:44 PM, Anupam <an...@gmail.com> wrote:
> I have not written or looked at a lot of code which uses ITask so my
> comments are more in-general.
>
> When designing an interface, conflicting choices can be resolved by a
> general principle of prioritizing ease of use of common scenario over
> ease of implementation or obscure scenario.
> If we go with current proposal the general code which used ITask would
> change from:
>
> using(var task = CreateTask())
> {
> task.Foo();
> }
>
> to
>
> var task = CreateTask();
>
> using(task as IDisposable) // null is handled by the compiler
> transformation.
> {
> task.Foo();
> }
>
> or
>
> var task = CreateTask();
> task.Foo();
>
>
> The three approaches are all appropriate based on our expectation how
> tasks will be implemented and used.
> If most people would use third approach then it might make sense for
> some users of the interface to follow second pattern when needed.
> Looking at code within REEF code I support the approach of removing
> IDisposable from ITask.
>
> P.S.: In general, I have this problem with a few concepts in .NET
> where implementation detail bleeds into the interface. Another
> instance is async APIs returning Tasks (fundamental change in signature).
>
> On 19 November 2015 at 13:11, Markus Weimer <ma...@weimo.de> wrote:
>
> > On 2015-11-19 10:20, Boris Shulman wrote:
> > > Another way to do that is remove that from ITask but check if Task
> > > actually implements that interface and act accordingly. This way
> > > we will give the ability to declare Tasks as disposables when it
> > > is actually needed.
> >
> > I like that. It de-clutters the interface and maintains current behavior.
> >
> > Markus
> >
>
>
>
> --
> Anupam
> Bellevue, WA
> Ph: +1 (425)-777-5570
>
Re: Does ITask need to be IDisposable?
Posted by Markus Weimer <ma...@weimo.de>.
+1 on keeping `ITaskStop` for abstraction parity with Java.
Markus
On 2015-11-20 15:11, Boris Shulman wrote:
> +1 on Andrews proposal.
>
> -----Original Message-----
> From: "Andrew Chung" <af...@gmail.com>
> Sent: 11/20/2015 2:36 PM
> To: "dev@reef.incubator.apache.org" <de...@reef.incubator.apache.org>
> Subject: Re: Does ITask need to be IDisposable?
>
> +1
>
> On the other hand, should we also keep the `ITaskStop` event? This means
> that we will need to synchronously call the ITaskStop event handler before
> the Dispose method, if ITask also implements the ITaskStop event observer.
>
> Thanks,
> Andrew
>
> On Fri, Nov 20, 2015 at 2:07 PM, Markus Weimer <ma...@weimo.de> wrote:
>
>> On 2015-11-19 15:25, Boris Shulman wrote:
>>> Can we agree on doing: var task = CreateTask();
>>>
>>> using(task as IDisposable) // null is handled by the compiler
>>> transformation. { task.Foo(); }
>>>
>>> ?
>>
>> Anupam just explained it to me, let me repeat for the benefit of the
>> less C# literate: `task as IDisposable` evaluates to `null` if `task`
>> does not implement `IDisposable`. Also `using(null){block;}` evaluates
>> to `block;`.
>>
>> Hence, C# handles this rather nicely for us. Hence, my proposal is:
>>
>> * Use the above code inside the Evaluator. It makes sure that
>> `.Dispose()` is called if the task implements `IDisposable`.
>>
>> * Remove `: IDisposable` from line 24 of `ITask.cs`
>>
>> Does that sound like a plan?
>>
>> Markus
>>
>
RE: Does ITask need to be IDisposable?
Posted by Boris Shulman <sh...@gmail.com>.
+1 on Andrews proposal.
-----Original Message-----
From: "Andrew Chung" <af...@gmail.com>
Sent: 11/20/2015 2:36 PM
To: "dev@reef.incubator.apache.org" <de...@reef.incubator.apache.org>
Subject: Re: Does ITask need to be IDisposable?
+1
On the other hand, should we also keep the `ITaskStop` event? This means
that we will need to synchronously call the ITaskStop event handler before
the Dispose method, if ITask also implements the ITaskStop event observer.
Thanks,
Andrew
On Fri, Nov 20, 2015 at 2:07 PM, Markus Weimer <ma...@weimo.de> wrote:
> On 2015-11-19 15:25, Boris Shulman wrote:
> > Can we agree on doing: var task = CreateTask();
> >
> > using(task as IDisposable) // null is handled by the compiler
> > transformation. { task.Foo(); }
> >
> > ?
>
> Anupam just explained it to me, let me repeat for the benefit of the
> less C# literate: `task as IDisposable` evaluates to `null` if `task`
> does not implement `IDisposable`. Also `using(null){block;}` evaluates
> to `block;`.
>
> Hence, C# handles this rather nicely for us. Hence, my proposal is:
>
> * Use the above code inside the Evaluator. It makes sure that
> `.Dispose()` is called if the task implements `IDisposable`.
>
> * Remove `: IDisposable` from line 24 of `ITask.cs`
>
> Does that sound like a plan?
>
> Markus
>
Re: Does ITask need to be IDisposable?
Posted by Andrew Chung <af...@gmail.com>.
+1
On the other hand, should we also keep the `ITaskStop` event? This means
that we will need to synchronously call the ITaskStop event handler before
the Dispose method, if ITask also implements the ITaskStop event observer.
Thanks,
Andrew
On Fri, Nov 20, 2015 at 2:07 PM, Markus Weimer <ma...@weimo.de> wrote:
> On 2015-11-19 15:25, Boris Shulman wrote:
> > Can we agree on doing: var task = CreateTask();
> >
> > using(task as IDisposable) // null is handled by the compiler
> > transformation. { task.Foo(); }
> >
> > ?
>
> Anupam just explained it to me, let me repeat for the benefit of the
> less C# literate: `task as IDisposable` evaluates to `null` if `task`
> does not implement `IDisposable`. Also `using(null){block;}` evaluates
> to `block;`.
>
> Hence, C# handles this rather nicely for us. Hence, my proposal is:
>
> * Use the above code inside the Evaluator. It makes sure that
> `.Dispose()` is called if the task implements `IDisposable`.
>
> * Remove `: IDisposable` from line 24 of `ITask.cs`
>
> Does that sound like a plan?
>
> Markus
>
Re: Does ITask need to be IDisposable?
Posted by Markus Weimer <ma...@weimo.de>.
On 2015-11-19 15:25, Boris Shulman wrote:
> Can we agree on doing: var task = CreateTask();
>
> using(task as IDisposable) // null is handled by the compiler
> transformation. { task.Foo(); }
>
> ?
Anupam just explained it to me, let me repeat for the benefit of the
less C# literate: `task as IDisposable` evaluates to `null` if `task`
does not implement `IDisposable`. Also `using(null){block;}` evaluates
to `block;`.
Hence, C# handles this rather nicely for us. Hence, my proposal is:
* Use the above code inside the Evaluator. It makes sure that
`.Dispose()` is called if the task implements `IDisposable`.
* Remove `: IDisposable` from line 24 of `ITask.cs`
Does that sound like a plan?
Markus
Re: Does ITask need to be IDisposable?
Posted by Boris Shulman <sh...@gmail.com>.
Can we agree on doing:
var task = CreateTask();
using(task as IDisposable) // null is handled by the compiler
transformation.
{
task.Foo();
}
?
On Thu, Nov 19, 2015 at 1:44 PM, Anupam <an...@gmail.com> wrote:
> I have not written or looked at a lot of code which uses ITask so my
> comments are more in-general.
>
> When designing an interface, conflicting choices can be resolved by a
> general principle of prioritizing ease of use of common scenario over ease
> of implementation or obscure scenario.
> If we go with current proposal the general code which used ITask would
> change from:
>
> using(var task = CreateTask())
> {
> task.Foo();
> }
>
> to
>
> var task = CreateTask();
>
> using(task as IDisposable) // null is handled by the compiler
> transformation.
> {
> task.Foo();
> }
>
> or
>
> var task = CreateTask();
> task.Foo();
>
>
> The three approaches are all appropriate based on our expectation how tasks
> will be implemented and used.
> If most people would use third approach then it might make sense for some
> users of the interface to follow second pattern when needed.
> Looking at code within REEF code I support the approach of removing
> IDisposable from ITask.
>
> P.S.: In general, I have this problem with a few concepts in .NET where
> implementation detail bleeds into the interface. Another instance is async
> APIs returning Tasks (fundamental change in signature).
>
> On 19 November 2015 at 13:11, Markus Weimer <ma...@weimo.de> wrote:
>
> > On 2015-11-19 10:20, Boris Shulman wrote:
> > > Another way to do that is remove that from ITask but check if Task
> > > actually implements that interface and act accordingly. This way we
> > > will give the ability to declare Tasks as disposables when it is
> > > actually needed.
> >
> > I like that. It de-clutters the interface and maintains current behavior.
> >
> > Markus
> >
>
>
>
> --
> Anupam
> Bellevue, WA
> Ph: +1 (425)-777-5570
>
Re: Does ITask need to be IDisposable?
Posted by Anupam <an...@gmail.com>.
I have not written or looked at a lot of code which uses ITask so my
comments are more in-general.
When designing an interface, conflicting choices can be resolved by a
general principle of prioritizing ease of use of common scenario over ease
of implementation or obscure scenario.
If we go with current proposal the general code which used ITask would
change from:
using(var task = CreateTask())
{
task.Foo();
}
to
var task = CreateTask();
using(task as IDisposable) // null is handled by the compiler
transformation.
{
task.Foo();
}
or
var task = CreateTask();
task.Foo();
The three approaches are all appropriate based on our expectation how tasks
will be implemented and used.
If most people would use third approach then it might make sense for some
users of the interface to follow second pattern when needed.
Looking at code within REEF code I support the approach of removing
IDisposable from ITask.
P.S.: In general, I have this problem with a few concepts in .NET where
implementation detail bleeds into the interface. Another instance is async
APIs returning Tasks (fundamental change in signature).
On 19 November 2015 at 13:11, Markus Weimer <ma...@weimo.de> wrote:
> On 2015-11-19 10:20, Boris Shulman wrote:
> > Another way to do that is remove that from ITask but check if Task
> > actually implements that interface and act accordingly. This way we
> > will give the ability to declare Tasks as disposables when it is
> > actually needed.
>
> I like that. It de-clutters the interface and maintains current behavior.
>
> Markus
>
--
Anupam
Bellevue, WA
Ph: +1 (425)-777-5570
Re: Does ITask need to be IDisposable?
Posted by Markus Weimer <ma...@weimo.de>.
On 2015-11-19 10:20, Boris Shulman wrote:
> Another way to do that is remove that from ITask but check if Task
> actually implements that interface and act accordingly. This way we
> will give the ability to declare Tasks as disposables when it is
> actually needed.
I like that. It de-clutters the interface and maintains current behavior.
Markus
Re: Does ITask need to be IDisposable?
Posted by Boris Shulman <sh...@gmail.com>.
Another way to do that is remove that from ITask but check if Task actually
implements that interface and act accordingly. This way we will give the
ability to declare Tasks as disposables when it is actually needed.
On Thu, Nov 19, 2015 at 10:16 AM, Boris Shulman <sh...@gmail.com> wrote:
> But having Dispose method is a standard c# way to deal with closing
> resources. Why do we need to remove that? Also with Dispose it easier to
> guarantee that it will be actually invoked.
>
> On Thu, Nov 19, 2015 at 10:04 AM, Andrew Chung <af...@gmail.com>
> wrote:
>
>> There is an ITaskStop event for that purpose instead.
>>
>> Thanks,
>> Andrew
>>
>> On Thu, Nov 19, 2015 at 9:49 AM, Boris Shulman <sh...@gmail.com>
>> wrote:
>>
>> > I think it useful to have it disposable. This way the task has a chance
>> to
>> > run some cleanup code on unhandled exceptions in the task code. I
>> actually
>> > think we added it later specifically for this.
>> >
>> > On Thu, Nov 19, 2015 at 8:55 AM, Markus Weimer <ma...@weimo.de> wrote:
>> >
>> > > Hi,
>> > >
>> > > currently, `ITask` is `IDisposable`, all our tasks need to implement a
>> > > `Dispose()` method. I'm not convinced this needs to be the case:
>> > >
>> > > First of all, it clutters the code of many REEF Tasks, which is
>> > especially
>> > > annoying in our Examples. Secondly, we raise an event after a Task
>> > > finishes, which makes `Dispose()` redundant. Also, the `Call()` method
>> > can
>> > > just do whatever `Dispose()` would do at its end.
>> > >
>> > > Hence, I propose to `[Obsolete]` `ITask.Dispose()` in 0.14 and to plan
>> > for
>> > > its removal in 0.15.
>> > >
>> > > Thoughts?
>> > >
>> > > Markus
>> > >
>> >
>>
>
>
Re: Does ITask need to be IDisposable?
Posted by Boris Shulman <sh...@gmail.com>.
But having Dispose method is a standard c# way to deal with closing
resources. Why do we need to remove that? Also with Dispose it easier to
guarantee that it will be actually invoked.
On Thu, Nov 19, 2015 at 10:04 AM, Andrew Chung <af...@gmail.com> wrote:
> There is an ITaskStop event for that purpose instead.
>
> Thanks,
> Andrew
>
> On Thu, Nov 19, 2015 at 9:49 AM, Boris Shulman <sh...@gmail.com> wrote:
>
> > I think it useful to have it disposable. This way the task has a chance
> to
> > run some cleanup code on unhandled exceptions in the task code. I
> actually
> > think we added it later specifically for this.
> >
> > On Thu, Nov 19, 2015 at 8:55 AM, Markus Weimer <ma...@weimo.de> wrote:
> >
> > > Hi,
> > >
> > > currently, `ITask` is `IDisposable`, all our tasks need to implement a
> > > `Dispose()` method. I'm not convinced this needs to be the case:
> > >
> > > First of all, it clutters the code of many REEF Tasks, which is
> > especially
> > > annoying in our Examples. Secondly, we raise an event after a Task
> > > finishes, which makes `Dispose()` redundant. Also, the `Call()` method
> > can
> > > just do whatever `Dispose()` would do at its end.
> > >
> > > Hence, I propose to `[Obsolete]` `ITask.Dispose()` in 0.14 and to plan
> > for
> > > its removal in 0.15.
> > >
> > > Thoughts?
> > >
> > > Markus
> > >
> >
>
Re: Does ITask need to be IDisposable?
Posted by Andrew Chung <af...@gmail.com>.
There is an ITaskStop event for that purpose instead.
Thanks,
Andrew
On Thu, Nov 19, 2015 at 9:49 AM, Boris Shulman <sh...@gmail.com> wrote:
> I think it useful to have it disposable. This way the task has a chance to
> run some cleanup code on unhandled exceptions in the task code. I actually
> think we added it later specifically for this.
>
> On Thu, Nov 19, 2015 at 8:55 AM, Markus Weimer <ma...@weimo.de> wrote:
>
> > Hi,
> >
> > currently, `ITask` is `IDisposable`, all our tasks need to implement a
> > `Dispose()` method. I'm not convinced this needs to be the case:
> >
> > First of all, it clutters the code of many REEF Tasks, which is
> especially
> > annoying in our Examples. Secondly, we raise an event after a Task
> > finishes, which makes `Dispose()` redundant. Also, the `Call()` method
> can
> > just do whatever `Dispose()` would do at its end.
> >
> > Hence, I propose to `[Obsolete]` `ITask.Dispose()` in 0.14 and to plan
> for
> > its removal in 0.15.
> >
> > Thoughts?
> >
> > Markus
> >
>
Re: Does ITask need to be IDisposable?
Posted by Boris Shulman <sh...@gmail.com>.
I think it useful to have it disposable. This way the task has a chance to
run some cleanup code on unhandled exceptions in the task code. I actually
think we added it later specifically for this.
On Thu, Nov 19, 2015 at 8:55 AM, Markus Weimer <ma...@weimo.de> wrote:
> Hi,
>
> currently, `ITask` is `IDisposable`, all our tasks need to implement a
> `Dispose()` method. I'm not convinced this needs to be the case:
>
> First of all, it clutters the code of many REEF Tasks, which is especially
> annoying in our Examples. Secondly, we raise an event after a Task
> finishes, which makes `Dispose()` redundant. Also, the `Call()` method can
> just do whatever `Dispose()` would do at its end.
>
> Hence, I propose to `[Obsolete]` `ITask.Dispose()` in 0.14 and to plan for
> its removal in 0.15.
>
> Thoughts?
>
> Markus
>