MIME-Version: 1.0
In-Reply-To:
 <CADv3eywb0tguYowTAK5Ag9YZ48zFZA0QJVNEj_cZcCpr-76Bbg@mail.gmail.com>
References: <1335185032-13075-1-git-send-email-felipe.contreras@gmail.com>
	<CADv3eywAvyMuh3vWLwyuf0Ui_kskwp9875pGxCR1GTm7deN9Pg@mail.gmail.com>
	<CAMP44s3SyU4WVV0_McHWseNL=jmMnAXO2EdZK4Xk-wrCHPVD8A@mail.gmail.com>
	<CADv3eywb0tguYowTAK5Ag9YZ48zFZA0QJVNEj_cZcCpr-76Bbg@mail.gmail.com>
Date: Tue, 24 Apr 2012 01:45:23 +0300
Message-ID:
 <CAMP44s0=m+PmVBdVytHaYujpaZu=2WH+1F_VoMzpfXH+SS_ZmQ@mail.gmail.com>
Subject: Re: [PATCH] ruby: make sure the database is closed
From: Felipe Contreras <felipe.contreras@gmail.com>
To: Ali Polatel <alip@exherbo.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Cc: notmuch@notmuchmail.org
Precedence: list

On Mon, Apr 23, 2012 at 11:43 PM, Ali Polatel <alip@exherbo.org> wrote:
> 2012/4/23 Felipe Contreras <felipe.contreras@gmail.com>:
>> On Mon, Apr 23, 2012 at 5:04 PM, Ali Polatel <alip@exherbo.org> wrote:
>>
>>> I'd rather not do this.
>>> Please read: http://comments.gmane.org/gmane.comp.lang.ruby.general/320=
324
>>
>> OK, I've read this.. So?
>
> You are one step close to what I thought you had thought.

I don't parse that. I meant that _now_ I've read it.

>> The order in which Ruby's garbage-collector frees the database and
>> other objects is irrelevant, because with this patch we are not
>> manually freeing other objects, only the database.
>
> What I wanted was to make all objects "depending" on the database to be u=
nusable
> once the database object is freed. Seeing that's not practically easy
> I decided to leave
> it to the user.

Well, sure, but if the user has problems, the user can keep the
database object around.

Personally I don't see why an object, like say a query would remain
working correctly after the database is gone, either by calling
.close() directly, or just loosing the pointer to the original object.
I don't think users would expect that, or, even if they somehow found
it useful, that most likely would be very seldom, and hardly worth
worrying about it.

>> Sure, it's _better_ if the user calls close(), even better if it's
>> inside an 'ensure', and even better if blocks are used (which I am
>> using in most cases), but that's not *required*.
>
> If you have such a use case, I'm fine with that patch.
> I might push it in the next few days or get someone else to push it.

I did at some point, not sure if I do now. But that's beside the
point, the point is that it really does happen.

>> The user might just do:
>>
>> def foo
>> =C2=A0db =3D Notmuch::Database.new($db_name, :mode =3D> Notmuch::MODE_RE=
AD_WRITE)
>> end
>>
>> That's perfectly fine in Ruby (although not ideal), since 'db' will
>> get garbage-collected. But nobody will be able to use the database
>> again until that process is killed.
>>
>> You think that's correct?
>
> Yes that is correct. I have not thought about this.
> I'd say it's a partial misunderstanding on my part due to lack of
> (and/or too much) vodka.

Well, there's only two options.

a) This works:

  def foo
    db =3D Notmuch::Database.new($db_name, :mode =3D> Notmuch::MODE_READ_WR=
ITE)
  end

(as in; the database gets closed eventually)

b) This works:

  def start_query(search)
    db =3D Notmuch::Database.new($db_name)
    return db.query(search)
  end

(as in; the query returned would keep working properly)

I personally don't see why anybody would want b), and if they have
problems with code like that, I think it's obvious to see why.

I think we should go for a) and thus apply the patch.

Cheers.

--=20
Felipe Contreras