You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Neil Conway <ne...@gmail.com> on 2016/03/30 01:24:05 UTC
Re: [1/6] mesos git commit: Fixed a memory leak in the scheduler driver.
On Tue, Mar 29, 2016 at 7:19 PM, <vi...@apache.org> wrote:
> --- a/src/sched/sched.cpp
> +++ b/src/sched/sched.cpp
> @@ -1808,6 +1808,10 @@ MesosSchedulerDriver::~MesosSchedulerDriver()
> delete process;
> }
>
> + if (credential != NULL) {
> + delete credential;
> + }
`delete` of a NULL pointer is safe, so I would vote for removing the `if`.
Neil
Re: [1/6] mesos git commit: Fixed a memory leak in the scheduler driver.
Posted by Benjamin Mahler <bm...@apache.org>.
On Wed, Mar 30, 2016 at 2:17 PM, Neil Conway <ne...@gmail.com> wrote:
> On Wed, Mar 30, 2016 at 4:57 PM, Benjamin Mahler <bm...@apache.org>
> wrote:
> > Yikes! (3) being not true to me means that I needed non-local reasoning
> to
> > determine the optionality.
>
> Sorry: to clarify, I didn't mean "there is not always a latch" in the
> code in question. I meant: "writing 'delete latch;' is not a good way
> to imply that there is always a latch, because that code works fine if
> latch is optional."
> > int fd = -1;
> > ...
> > if (something) { fd = open(); }
> > ...
> > if (fd != -1) { close(fd); }
> >
> > Is there something fundamentally different about new/delete?
>
> To me, `if (ptr != NULL) { delete ptr; }` is distracting, because the
> fact that deleting a null pointer is well-defined is very well known.
>
Well close(-1) is also well-defined, and isn't "very well known" arguable?
I did not know this off the top of my head but I could guess it, and Anand
apparently learned this recently when I chatted with him. One could say
that close(-1) very well known to be a no-op, at least it is to me, but why
make the reader have to think about this kind of detail?
> So I immediately see that and wonder why there is obviously redundant
> code. In contrast, close(-1) not having some subtle overloaded meaning
> (like kill(-1) does) isn't as obvious.
>
> > Generally we use options for optionality, however we've occasionally
> > avoided Option<T*> in favor of T* out of convenience. For these
> Option<T*>
> > variables masquerading as T* variables, it would be great to keep the
> > optionality checks to help the reader intuit the optionality, or convert
> to
> > something better than just naked deletes.
>
> I definitely think there is value in distinguishing between optional
> and non-optional pointer values, but I don't think no-op statements
> like `if (ptr) { delete ptr; }` are the best way to do that. +1 on
> using something better than just naked deletes!
>
I think we're agreed on the ideal. For the proposal of changing the
existing approach to deletion of pointers, I can see a lot of code using
null guards around deletes and I'm not convinced that removing the guards
makes life easier for the reader in many cases. If we wanted to remove
them, we should propose such a change first and vote on it. Is there a lot
of code relying on naked deletes?
>
> Neil
>
Re: [1/6] mesos git commit: Fixed a memory leak in the scheduler driver.
Posted by Benjamin Mahler <bm...@apache.org>.
On Wed, Mar 30, 2016 at 2:17 PM, Neil Conway <ne...@gmail.com> wrote:
> On Wed, Mar 30, 2016 at 4:57 PM, Benjamin Mahler <bm...@apache.org>
> wrote:
> > Yikes! (3) being not true to me means that I needed non-local reasoning
> to
> > determine the optionality.
>
> Sorry: to clarify, I didn't mean "there is not always a latch" in the
> code in question. I meant: "writing 'delete latch;' is not a good way
> to imply that there is always a latch, because that code works fine if
> latch is optional."
> > int fd = -1;
> > ...
> > if (something) { fd = open(); }
> > ...
> > if (fd != -1) { close(fd); }
> >
> > Is there something fundamentally different about new/delete?
>
> To me, `if (ptr != NULL) { delete ptr; }` is distracting, because the
> fact that deleting a null pointer is well-defined is very well known.
>
Well close(-1) is also well-defined, and isn't "very well known" arguable?
I did not know this off the top of my head but I could guess it, and Anand
apparently learned this recently when I chatted with him. One could say
that close(-1) very well known to be a no-op, at least it is to me, but why
make the reader have to think about this kind of detail?
> So I immediately see that and wonder why there is obviously redundant
> code. In contrast, close(-1) not having some subtle overloaded meaning
> (like kill(-1) does) isn't as obvious.
>
> > Generally we use options for optionality, however we've occasionally
> > avoided Option<T*> in favor of T* out of convenience. For these
> Option<T*>
> > variables masquerading as T* variables, it would be great to keep the
> > optionality checks to help the reader intuit the optionality, or convert
> to
> > something better than just naked deletes.
>
> I definitely think there is value in distinguishing between optional
> and non-optional pointer values, but I don't think no-op statements
> like `if (ptr) { delete ptr; }` are the best way to do that. +1 on
> using something better than just naked deletes!
>
I think we're agreed on the ideal. For the proposal of changing the
existing approach to deletion of pointers, I can see a lot of code using
null guards around deletes and I'm not convinced that removing the guards
makes life easier for the reader in many cases. If we wanted to remove
them, we should propose such a change first and vote on it. Is there a lot
of code relying on naked deletes?
>
> Neil
>
Re: [1/6] mesos git commit: Fixed a memory leak in the scheduler driver.
Posted by Neil Conway <ne...@gmail.com>.
On Wed, Mar 30, 2016 at 4:57 PM, Benjamin Mahler <bm...@apache.org> wrote:
> Yikes! (3) being not true to me means that I needed non-local reasoning to
> determine the optionality.
Sorry: to clarify, I didn't mean "there is not always a latch" in the
code in question. I meant: "writing 'delete latch;' is not a good way
to imply that there is always a latch, because that code works fine if
latch is optional."
> int fd = -1;
> ...
> if (something) { fd = open(); }
> ...
> if (fd != -1) { close(fd); }
>
> Is there something fundamentally different about new/delete?
To me, `if (ptr != NULL) { delete ptr; }` is distracting, because the
fact that deleting a null pointer is well-defined is very well known.
So I immediately see that and wonder why there is obviously redundant
code. In contrast, close(-1) not having some subtle overloaded meaning
(like kill(-1) does) isn't as obvious.
> Generally we use options for optionality, however we've occasionally
> avoided Option<T*> in favor of T* out of convenience. For these Option<T*>
> variables masquerading as T* variables, it would be great to keep the
> optionality checks to help the reader intuit the optionality, or convert to
> something better than just naked deletes.
I definitely think there is value in distinguishing between optional
and non-optional pointer values, but I don't think no-op statements
like `if (ptr) { delete ptr; }` are the best way to do that. +1 on
using something better than just naked deletes!
Neil
Re: [1/6] mesos git commit: Fixed a memory leak in the scheduler driver.
Posted by Benjamin Mahler <bm...@apache.org>.
Yikes! (3) being not true to me means that I needed non-local reasoning to
determine the optionality.
I reason about symmetric operations like new/delete in a similar way to
other symmetric operations like open/close. Even though close(-1) returns
EBADF and doesn't do something bad, it's surprising to the reader if
there's a naked call to close when there may or may not be a call to open.
int fd = -1;
...
if (something) { fd = open(); }
...
close(fd);
It seems clever to rely on close(-1) not doing something bad, and we're
likely to mislead the reader when they encounter the close statement
separate from the other logic. Whereas if I saw the following, I don't need
non-local reasoning to determine that the fd may not represent an open file:
int fd = -1;
...
if (something) { fd = open(); }
...
if (fd != -1) { close(fd); }
Is there something fundamentally different about new/delete?
Generally we use options for optionality, however we've occasionally
avoided Option<T*> in favor of T* out of convenience. For these Option<T*>
variables masquerading as T* variables, it would be great to keep the
optionality checks to help the reader intuit the optionality, or convert to
something better than just naked deletes.
Re: [1/6] mesos git commit: Fixed a memory leak in the scheduler driver.
Posted by Benjamin Mahler <bm...@apache.org>.
Yikes! (3) being not true to me means that I needed non-local reasoning to
determine the optionality.
I reason about symmetric operations like new/delete in a similar way to
other symmetric operations like open/close. Even though close(-1) returns
EBADF and doesn't do something bad, it's surprising to the reader if
there's a naked call to close when there may or may not be a call to open.
int fd = -1;
...
if (something) { fd = open(); }
...
close(fd);
It seems clever to rely on close(-1) not doing something bad, and we're
likely to mislead the reader when they encounter the close statement
separate from the other logic. Whereas if I saw the following, I don't need
non-local reasoning to determine that the fd may not represent an open file:
int fd = -1;
...
if (something) { fd = open(); }
...
if (fd != -1) { close(fd); }
Is there something fundamentally different about new/delete?
Generally we use options for optionality, however we've occasionally
avoided Option<T*> in favor of T* out of convenience. For these Option<T*>
variables masquerading as T* variables, it would be great to keep the
optionality checks to help the reader intuit the optionality, or convert to
something better than just naked deletes.
Re: [1/6] mesos git commit: Fixed a memory leak in the scheduler driver.
Posted by Neil Conway <ne...@gmail.com>.
(3) is not true, though: as written, there may or may not be a latch
(the code works correctly correct either way). Using an operation
(delete) that works fine for null pointers as a way to imply that a
pointer is NOT null does not seem like the best arrangement.
As you say, there should be better ways to communicate to the reader
that `process` and `credential` are optional but `latch` is not. For
example:
* encoding this into the type system using Owned / Option / etc.
* CHECKs (e.g., CHECK that `latch` is not NULL).
* comments
Neil
On Wed, Mar 30, 2016 at 3:24 PM, Benjamin Mahler <bm...@apache.org> wrote:
> I'm not sure the null check was in place for the safety of deletion so much
> as to make the code easier to reason about:
>
> if (process != NULL) {
> terminate(process);
> wait(process);
> delete process;
> }
>
> if (credential != NULL) {
> delete credential;
> }
>
> delete latch;
>
> Here, (1) there's sometimes a process, (2) theres's sometimes a credential,
> (3) there's always a latch. Without the null check for credential, the
> optionality seems less clear to the reader (more non-local reasoning
> required). Arguably we could use Owned or Options of pointers here, but in
> its current form I would opt to leave the NULL check in to help the reader.
>
> On Tue, Mar 29, 2016 at 4:39 PM, Klaus Ma <kl...@hotmail.com> wrote:
>
>> +1
>>
>> Refer to this doc for the detail of deleting null:
>> http://www.cplusplus.com/reference/new/operator%20delete/ <
>> http://www.cplusplus.com/reference/new/operator%20delete/>
>>
>> Thanks
>> Klaus
>>
>> > On Mar 30, 2016, at 07:24, Neil Conway <ne...@gmail.com> wrote:
>> >
>> > On Tue, Mar 29, 2016 at 7:19 PM, <vi...@apache.org> wrote:
>> >> --- a/src/sched/sched.cpp
>> >> +++ b/src/sched/sched.cpp
>> >> @@ -1808,6 +1808,10 @@ MesosSchedulerDriver::~MesosSchedulerDriver()
>> >> delete process;
>> >> }
>> >>
>> >> + if (credential != NULL) {
>> >> + delete credential;
>> >> + }
>> >
>> > `delete` of a NULL pointer is safe, so I would vote for removing the
>> `if`.
>> >
>> > Neil
>>
>>
Re: [1/6] mesos git commit: Fixed a memory leak in the scheduler driver.
Posted by Benjamin Mahler <bm...@apache.org>.
I'm not sure the null check was in place for the safety of deletion so much
as to make the code easier to reason about:
if (process != NULL) {
terminate(process);
wait(process);
delete process;
}
if (credential != NULL) {
delete credential;
}
delete latch;
Here, (1) there's sometimes a process, (2) theres's sometimes a credential,
(3) there's always a latch. Without the null check for credential, the
optionality seems less clear to the reader (more non-local reasoning
required). Arguably we could use Owned or Options of pointers here, but in
its current form I would opt to leave the NULL check in to help the reader.
On Tue, Mar 29, 2016 at 4:39 PM, Klaus Ma <kl...@hotmail.com> wrote:
> +1
>
> Refer to this doc for the detail of deleting null:
> http://www.cplusplus.com/reference/new/operator%20delete/ <
> http://www.cplusplus.com/reference/new/operator%20delete/>
>
> Thanks
> Klaus
>
> > On Mar 30, 2016, at 07:24, Neil Conway <ne...@gmail.com> wrote:
> >
> > On Tue, Mar 29, 2016 at 7:19 PM, <vi...@apache.org> wrote:
> >> --- a/src/sched/sched.cpp
> >> +++ b/src/sched/sched.cpp
> >> @@ -1808,6 +1808,10 @@ MesosSchedulerDriver::~MesosSchedulerDriver()
> >> delete process;
> >> }
> >>
> >> + if (credential != NULL) {
> >> + delete credential;
> >> + }
> >
> > `delete` of a NULL pointer is safe, so I would vote for removing the
> `if`.
> >
> > Neil
>
>
Re: [1/6] mesos git commit: Fixed a memory leak in the scheduler driver.
Posted by Benjamin Mahler <bm...@apache.org>.
I'm not sure the null check was in place for the safety of deletion so much
as to make the code easier to reason about:
if (process != NULL) {
terminate(process);
wait(process);
delete process;
}
if (credential != NULL) {
delete credential;
}
delete latch;
Here, (1) there's sometimes a process, (2) theres's sometimes a credential,
(3) there's always a latch. Without the null check for credential, the
optionality seems less clear to the reader (more non-local reasoning
required). Arguably we could use Owned or Options of pointers here, but in
its current form I would opt to leave the NULL check in to help the reader.
On Tue, Mar 29, 2016 at 4:39 PM, Klaus Ma <kl...@hotmail.com> wrote:
> +1
>
> Refer to this doc for the detail of deleting null:
> http://www.cplusplus.com/reference/new/operator%20delete/ <
> http://www.cplusplus.com/reference/new/operator%20delete/>
>
> Thanks
> Klaus
>
> > On Mar 30, 2016, at 07:24, Neil Conway <ne...@gmail.com> wrote:
> >
> > On Tue, Mar 29, 2016 at 7:19 PM, <vi...@apache.org> wrote:
> >> --- a/src/sched/sched.cpp
> >> +++ b/src/sched/sched.cpp
> >> @@ -1808,6 +1808,10 @@ MesosSchedulerDriver::~MesosSchedulerDriver()
> >> delete process;
> >> }
> >>
> >> + if (credential != NULL) {
> >> + delete credential;
> >> + }
> >
> > `delete` of a NULL pointer is safe, so I would vote for removing the
> `if`.
> >
> > Neil
>
>
Re: [1/6] mesos git commit: Fixed a memory leak in the scheduler driver.
Posted by Klaus Ma <kl...@hotmail.com>.
+1
Refer to this doc for the detail of deleting null: http://www.cplusplus.com/reference/new/operator%20delete/ <http://www.cplusplus.com/reference/new/operator%20delete/>
Thanks
Klaus
> On Mar 30, 2016, at 07:24, Neil Conway <ne...@gmail.com> wrote:
>
> On Tue, Mar 29, 2016 at 7:19 PM, <vi...@apache.org> wrote:
>> --- a/src/sched/sched.cpp
>> +++ b/src/sched/sched.cpp
>> @@ -1808,6 +1808,10 @@ MesosSchedulerDriver::~MesosSchedulerDriver()
>> delete process;
>> }
>>
>> + if (credential != NULL) {
>> + delete credential;
>> + }
>
> `delete` of a NULL pointer is safe, so I would vote for removing the `if`.
>
> Neil