Skip to content

Add failing test for perl 5.17.6 hash seed randomization #2

Closed
wants to merge 2 commits into from

5 participants

@robinsmidsrod

When Data::Visitor::Callback->visit() is used on a Moose type constraint, it randomly triggers a test failure on perl 5.17.6. On perl 5.16.2 it never triggers a test failure.

robinsmidsrod added some commits Dec 2, 2012
@robinsmidsrod robinsmidsrod Add missing Test::MockObject::Extends to prereqs a0cf940
@robinsmidsrod robinsmidsrod Add failing test for perl 5.17.6 hash seed randomization
When Data::Visitor::Callback->visit() is used on a Moose
type constraint, it randomly triggers a test failure on
perl 5.17.6. On perl 5.16.2 it never triggers a test failure.
878832a
@mauke
mauke commented Dec 3, 2012

I ran the file with DATA_VISITOR_DEBUG=1 perl -MCarp::Always bug.t. At https://gist.github.com/4194050 you can see two logs (a failing and a successful run) filtered through grep 'flow: visit_hash_entry'. As you can see, the key order is different.

As far as I understand the problem, the spurious second Moose::Meta::TypeConstraint::Class object is actually a clone of the original created by Data::Visitor::Callback itself. It starts life as a plain hashref, then gets blessed, then gets visited (?!).

I also think Data::Visitor and Data::Visitor::Callback have different ideas about how the return value of callbacks is supposed to be used (and this might be the cause of the problem), but I'm not sure about that because the documentation is rather sparse in this area. :-/

@doy
doy commented on 878832a Jun 24, 2013

So I fixed this in 1a1ddc9 (and Data::Visitor 0.29), but the reason this hasn't been noticed before is because your code isn't really written correctly to begin with. The class callbacks don't return "a bunch of stuff that should also be visited", they return "the thing to pretend i visited at this point in the traversal instead of what i found", for when you actually use the return value of ->visit. Returning some completely unrelated object isn't really using it as intended, and doesn't actually do anything useful. The reason you were getting a traversal that looked reasonable to begin with was just because of the object => 'visit_ref' part, not the class callback part - your callbacks here for ::Parameterized and ::Union aren't actually doing anything other than confusing the traversal. What you probably want is something more like this:

Data::Visitor::Callback->new({
    'Moose::Meta::TypeConstraint::Class'         => sub { push @classes, $_[1]->class; return $_[1]; },
    'Moose::Meta::TypeConstraint::Parameterized' => 'visit_ref',
    # or, if you actually want to suppress the rest of the stuff in ::Parameterized,
    # 'Moose::Meta::TypeConstraint::Parameterized' => sub { $_[0]->visit_ref({ type_parameter => $_[1]->type_parameter }); },
})->visit($tc);

Does that make sense?

@doy
doy commented Jun 24, 2013

Also, for what it's worth, my fork of the repository is should probably be considered canonical at this point, since I don't think nothingmuch is planning on doing much with it anymore(:

@robinsmidsrod

@doy I've actually removed the need for Data::Visitor::Callback in my code. See robinsmidsrod/XML-Rabbit@7fcaf2f for details on how I worked around the issue.

Thanks for the note that I was actually doing it wrong. I'm not sure I fully grasp the details, but if I ever end up looking at this again I'll dive in an figure it out. I'm fairly certain you're correct and I'm wrong.

Thanks for the heads up on where to submit bug reports from now on.

@robinsmidsrod robinsmidsrod reopened this Jul 2, 2013
@karenetheridge

I think this ticket can be closed now?

@nothingmuch nothingmuch closed this Jul 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.