develooper Front page | perl.perl5.porters | Postings from September 2014

Re: Hash::Util and read-only values

Thread Previous | Thread Next
From:
demerphq
Date:
September 20, 2014 03:03
Subject:
Re: Hash::Util and read-only values
Message ID:
CANgJU+VJ_u=d44wVNL9BAPrHJFcHasz9MUUbcnGoTuJRPSefPg@mail.gmail.com
On 20 September 2014 03:11, Father Chrysostomos <sprout@cpan.org> wrote:

> Yves Orton wrote:
> > On 20 September 2014 00:48, Father Chrysostomos <sprout cpan.org> wrote:
> >
> > > With Hash::Util::hv_store and Hash::Util::unlock_value, it is possible
> > > to turn off the read-only flag of any scalar.
> > >
> > > There are cases where the core uses this flag to protect things that
> > > must not be modified.  So we need to stop Hash::Util from being able
> > > to do this.
> > >
> >
> > This suggests a deeper design problem that I think needs to be thought
> > through independently of Hash::Util.
> >
> >
> > >
> > > (I bring this up, because with lvalue references it will become easier
> > > to do this by mistake.)
> > >
> > >
> > Yes, exactly, that is the problem.
> >
> >
> > > So we need to distinguish truly read-only values from values locked by
> > > Hash::Util.
> > >
> > >
> > Or alternatively we need to change things that are using the read-only
> bit
> > for different purposes.
> >
> >
> > > Hash::Util::unlock_value should croak for any read-only (as opposed to
> > > locked) value.
> >
> >
> > There is no difference between a locked value and a read only value.
>
> Currently there is none, but introducing such a distinction is easy.
>
>
Well if it interferes with peoples using of Internals::SvREADONLY() then
there is a problem.

People use that independent of Hash::Util.


> > > But should Hash::Util::lock_value croak for those, or
> > > just return?  I am inclined to think it should croak, as a successful
> > > lock_value implies that later unlock_value will work.
> > >
> > > Actually, since locking or unlocking an entire hash's values at
> > > once is possible, should (un)locking silently do nothing with read-
> > > only values?
> > >
> >
> > For me this has subject has XY problem written all over it.
>
> In this case the solution for Y would be identical to the solu-
> tion for X.
>
>
Is it? It sounds like you want to change Hash::Util, when I would argue
that Hash::Util simply uses Internals::SvREADONLY() and as such it is it,
and how the internals uses the readonly flag that is the problem.

IOW, I think if there is a problem with something turning off the read-only
flag then we should fix it so that the readonly flag doesnt matter.


> > I think we should solve the X problem, that we use the read only flag for
> > multiple purposes.
>
> In the perl core, the read-only flag only serves one purpose aside
> from Hash::Util.
>
>
Except that people think they can use Internals::SvREADONLY() directly and
they do.

Note XS_constant__make_const() in universal.c uses SvREADONLY_on()


> > Once we solve that there will be no Y problem, that Hash::Util does the
> > wrong thing.
>
> Since Hash::Util is the part of the core (if you will) that uses the
> flag differently, we do need to discuss how it will behave, since that
> is where the bugs in the design show through.
>
>
Again that isn't clear to me. Hash::Util simply uses
Internals::SvREADONLY(), anything that uses it, and stuff does, is an issue.


> > Independently, I think it is arguable that the locked hash implementation
> > is broken to the point of being nearly unusable.
> >
> > Specifically there is no way to lock a hash so you can not modify, delete
> > or add keys or values, but can still do things like fetch a missing key,
> > call exists on a missing key etc.
>
> I am not interested in solving this at present (if ever).
>
>
Well, that is fair enough. But I would say that if we arent going to solve
this we should rip out locked hashes entirely. They complicate and slow
down *all* hashes and hash accesses, and as far as I can tell, due to the
reasons I mention above, they are pretty much unused and unusable. We tried
to use them for a few applications at $work and basically it was a
nightmare.


> > Also related to this is the question of how to serialize a locked or read
> > only value?
>
> Or this.  (DDS already does make_ro though.  Hey, do you know about
> DDS?  I recommend it.)
>

I am somewhat familiar with the module, I actually know the author.[1]




>
> Okay, the two meanings for SvREADONLY are:
>
> 1) This value must not be modified and must be protected at all costs.
>    (Call this 'protected'.)
> 2) This value is temporarily read-only, at the user's request.
>    (Call this 'locked'.)
>
> Every use of SvREADONLY_on in the perl core, aside from Hash::Util, is
> for meaning 1.  Hash::Util's use of it is for meaning 2.
>
> (Before Hash::Util came along, no. 1 was the only meaning.)
>

Well except that out in wild people use the readonly flag also for purpose
2. Perhaps they shouldn't have, but they do. Sereal for instance has two
modes to mark every item it deserializes as read-only (but that doesnt work
well for the reasons I mentioned above about hashes).


>
> So what should Hash::Util::(un)lock_value do when it encounters a pro-
> tected value (assuming we have already introduced the distinction with
> internal flags)?  Croak?  Silently do nothing?
>
> Now, as for flag usage and XS macros, do we want to take into account
> the various CPAN modules that do SvREADONLY_on/off (which modules have
> always been buggy to some extent)?
>
> Do we want SvREADONLY to refer to locked values or to protected
> values?  The former would make such CPAN modules safer, but
> SvREADONLY_off would be effectively a silent no-op for protected val-
> ues.  The latter would have them continue to behave exactly as they do
> (dangerously).
>

IMO that boat has already sailed. SvREADONLY means locked, not protected.
Even if the core thinks otherwise. This is what I mean by an XY problem.
You are focusing on Hash::Util thinking it is the odd man out, I would
contend that Internals::SvREADONLY() has been exposed for over a decade,
and people are and have been using it in the "locked" form, so in fact the
internals use as "protected" is the odd man out. (Even if that feels weird
and is something that probably should not have happened.)


> BTW, here is one symptom of the problem:
>
> $ perl -MHash::Util=hv_store,unlock_value -MScalar::Util=weaken -e
> 'DESTROY{hv_store %h, k=>$_[0]; unlock_value %h, "k"; $x = $_[0]; weaken
> $_[0]; undef %h} $_ = bless[]'
> panic: magic_killbackrefs (flags=ff) during global destruction.
>
> I have managed to get variants of this to crash in the past.
>
>
Doesn't surprise me one bit. I think we need to separate out the internals
use of the readonly flag and NOT expose it AT ALL to the user, and leave
the current readonly flag alone, as it is in use in the wild and we arent
going to change that.

Yves

[1] For the uninitiated FC is being ironic here, as he knows I wrote DDS.
What he might not know is I also wrote/fixed a big chunk of Hash::Utils
recursive support.
-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

Thread Previous | Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About