Skip to content
This repository

no circular::require #217

Open
daxim opened this Issue December 05, 2011 · 22 comments

5 participants

Lars Dɪᴇᴄᴋᴏᴡ 迪拉斯 Chad Granum Michael G. Schwern Jesse Luehrs ben hengst
Lars Dɪᴇᴄᴋᴏᴡ 迪拉斯
Collaborator

http://p3rl.org/circular::require

Related: pragmatic stuff in #175, #202

Michael G. Schwern
Owner

Valuable idea, but it's global.

"This module works by overriding CORE::GLOBAL::require, and so other modules which do this may cause issues if they aren't written properly. This also means that the effect is global, but this is typically the most useful usage."

It should be possible to make at least the warning lexical. It will likely have to always override CORE::GLOBAL::require in order to trap the information. Alternatively, it could put code into @INC.

Chad Granum
Collaborator

I will also point out that until doy merges my pull request, or writes his own fix, that base.pm is badly broken bu circular::require (fails to load modules provided to it as arguments)

Jesse Luehrs
doy commented January 05, 2012

I don't think this makes a lot of sense as a lexical warning. The effect that it is tracking is inherently not limited to a lexical scope, so if you only enable no circular::require in some parts of a cycle but not others, you'll end up with the warning occurring sometimes and not others dependent on load order, which is kind of terrible (I have this same issue with perl's built-in "deep recursion" warning). That said, if you can come up with a patch that works and makes sense, I'll consider it, but I don't have a lot of interest in figuring this out myself.

Chad Granum
Collaborator

Cycle-Detect

Problem solved, but it involved creating an alternative module that takes a completely different approach:

Basically I was home sick in bed, I saw doy's response above and my mind started racing. I gave up on sleep and wrote this while suffering through my sick (not pleasant).

I decided not to usurp doy's module and wrote my approach into a new module. I also prefer the 'use' to enable, 'no' to disable non-pragma approach.

Features:

  • Only reports cycles started from a require in a package that uses it
  • Reports the use-cycle from start to end for easy tracing (see below)
  • Can be used as a one-off to load only one module with cycle detection
  • Can easily be used by perl5i to detect cycles on use-paths started from a perl5i importer
  • avoids the 'base.pm' bug all-together because of the approach taken.

Caveats:

  • Still overrides CORE::GLOBAL::require, but in a friendlier way

Example output:

    [Cycle Detection started in 'My::Package']
    Use Cycle Detected. Require Stack:
      * CycleA.pm
        CycleB.pm
        CycleC.pm
      * CycleA.pm
  • Shows the package that enabled cycle-detection
  • Shows the entire require stack
  • Uses asterisk to denote the cycled module
Jesse Luehrs
doy commented January 05, 2012

I really don't understand why there needs to be two modules for this, especially when the reason for circular::require not going into perl5i was because it wasn't lexical, which Cycle::Detect still isn't. The idea of printing the entire cycle is good (I've added this to circular::require 0.06), but I really can't agree that the way you're overriding CORE::GLOBAL::require is friendlier - it still does have the base.pm bug, you're not avoiding it in any way:

package Bar;
sub foo { }
package Foo;
use Cycle::Detect;
use base 'Bar'; # dies if Cycle::Detect is loaded, but not if it isn't

Can't we just get a single working implementation that makes everyone happy rather than splitting this into two implementations for no good reason?

Chad Granum
Collaborator

Ah, I misunderstood the base.pm bug.

There may not need to be 2 modules, I want to stress that when I made the decision to write my own yesterday I was home sick, having decided to go home sick when I realized my brain was not functioning properly. That said the motivations that led to that decision still stand. I realized the behavior I wanted would require radical refactoring or even rewriting of your code and I did not want to insult you by submitting a pull request that blew everything away. Of course my brain wasn't at 100% so I failed to notice that creating a new module didn't solve the 'not being an ass' problem.

Here are my list of essential requirements (combination of what I need at work and in my projects, and probably meet the requirements of perl5i):

  • If module A loads Module B and then Module C, and Module B loads circular::require, it should not bleed into Module C if Module C loads something that cycles.
  • Should issue a warning, but not be fatal
  • Should print the require cycle from start to finish
  • Should say which module is responsible for the warning (IE who loaded circular::require)
  • If a module that loads circular require, and then loads another module that also loads it, we should still only get one printout for the cycle.
  • Ability to use cycle-detection for a single load (use Detector Module @args;)
  • Specify which module was cycled, it might be hard to pick out in a large require-stack printout

Here is my list of nit-picks, these are things I dislike about circular:require but are completely me being picky:

  • I dislike that everything is stored in package level closures instead of being meta-data on the package that loaded circular::require
  • I really hate the hide/-hide argument (which is probably no longer necessary since the entire stack is printed)
  • using 'no' instead of 'use' to enable, and then use to disable is annoying, for instance in this entire message I had to say "when you load circular require", cause if I said "when you use circular require" It could be seen as "when I turn off the circular require module"
Jesse Luehrs
doy commented January 06, 2012

Version 0.07 of circular::require now makes the warning have dynamic scope. This makes a lot more sense than either lexical or package scoping, for what it's trying to do - for instance:

no circular::require;
require Foo;
use circular::require;
require Bar;

will report on cycles detected when Foo is loaded, but not when Bar is loaded (and if Foo loads modules with cycles that should be ignored, it can add a use circular::require statement itself to quiet them).

@schwern, is this close enough for you?

Chad Granum
Collaborator

The problem with this is that perl5i has to try to be lexical or package scope. Assuming we added it to perl5i it would look like this:

package Foo
use perl5i::latest;
# circular-require activated by perl5i
...
no perl5i::latest
# circular-require turned off

Currently perl5i does not require you to turn it off at the end of your package to keep things from bleeding out. Your fix does not prevent bleed into other modules yet to load, but not loaded by this package. Requiring people to add cleanup code at the end of the module to prevent action at a distance is a PITA. As well there is no sane way as of yet to inject code at the end of package compilation (which is why moose makes you call make_immutable by hand)

For perl5i to use circular::require it has to be either lexical or package scoped. My implementation of Cycle::Detect accomplishes this. @doy if you make circular::require package scoped I will add it to perl5i myself (I have a commit bit). IF you do that and add a mechanism to load a single package with detection, I will even consider deprecating Cycle::Detect.

Jesse Luehrs
doy commented January 06, 2012

This isn't true, that's not how dynamic scope (the scope that local uses) works. This won't have to be manually disabled to avoid leaking into other code (see t/dynamic3.t for an example).

Chad Granum
Collaborator

@doy, I am reading your implementation details again, I see you use %^H. So I think I was mistaken about how circular::Require works as of 0.07. Please correct me if I am wrong:

  • no circular::require - run in BEGIN, sets $^H{circular::require}
  • all other begins, including use statements run, cycles are detected
  • (optional) use circular::require to stop detection
  • last BEGIN block completes, %^H is restored to old values effectively turning off cycle detection
  • package scope executes, require() at this point does not detect cycles
  • next package loads, no cycle detection
Jesse Luehrs
doy commented January 06, 2012

No, that's not how %^H works. %^H is a special variable that is tied to each individual lexical scope, and which can be modified by things running at compilation time of that lexical scope. It can then be looked up later at runtime via caller. When a use/require is executed, it looks up the call stack to find the most recent call to enable or disable circular::require (by looking in %^H), and uses that to determine whether to print the warning. So:

{
    no circular::require;
    use Foo1; # warns
    require Foo2; # warns
    {
        use circular::require;
        use Bar1; # doesn't warn
        require Bar2; # doesn't warn
    }
    use Baz1; # warns
    require Baz2; # warns
}
use Quux1; # doesn't warn
require Quux2; # doesn't warn

Also, in that example, if Baz1 includes use circular::require, things it loads won't warn about cycles.

Chad Granum
Collaborator

I think we have a communications issue, your example and explanation above reflect how I thought %^H worked, yot you said I had it wrong.

The way circular::require works now seems sane to me, and most-correct of all the ideas/implementations between us so far. I feel comfortable integrating it into perl5i, I will see if I can make time to do that tonight or this weekend.

Jesse Luehrs
doy commented January 06, 2012

Also, with this behavior, I don't see a lot of a need for special syntax to load a single module with cycle detection, since you can just write:

{
    no circular::require;
    use Foo;
}
Chad Granum
Collaborator
Chad Granum exodist closed this issue from a commit January 07, 2012
Chad Granum Merge branch 'exodist/master' into HEAD
This introduces circular::require which will issue warnings if you load
modules that have circular dependencies from within a module that uses
perl5i.

Care has been taken to supress warnings that would issue fourth from
perl5i dependencies themselves which do have circular dependencies
(DateTime).

this fixes #217
6a51af0
Chad Granum exodist closed this in 6a51af0 January 07, 2012
Michael G. Schwern schwern reopened this January 22, 2012
Michael G. Schwern
Owner

Thanks for the work, but there's no docs and no tests. It can't be released in this state. I have to reopen.

Michael G. Schwern
Owner

There's an additional problem, it doubles load time. v2.9.1 loads in 170ms while master takes 300ms. 50ms of that is circular::require. The remaining 80ms is that DateTime is now loaded no matter what. Both are unacceptable increases in load time for the utility gotten.

I'd like to make a release to get the Google Code In work available, but I can't release in this state. I'm going to move the circular::require work into a branch (issue/217) so it can be worked on.

Michael G. Schwern schwern referenced this issue from a commit January 23, 2012
Michael G. Schwern Revert "Merge branch 'exodist/master' into HEAD"
This reverts commit 6a51af0, reversing
changes made to b572034.

This change doubled load time.  It will instead be moved into the issue/217 branch to
clear the way for release and so it can continue to be worked on.

For #217
e936a45
Chad Granum
Collaborator

Docs and tests I would be able to do. I was unable to disable circular::require for loading DateTime. I tried use circular::require, and circular::require->import() directly, however it seems disabling it only worked around a use statement... Which is probably reason enough to not release this yet.

As for the 50ms taken by circular require, you did not make it clear if the 50ms was a problem on its own, or only when combined with the other.

If load time is such a big deal my first thought was we should have a unit test for it... however it was immediately clear that load time is too subjective for a unit test, what takes 300ms on your macbook may take 100ms on my gentoo i7. My only thought is we can get a system/vhost somewhere that continuously runs the test suite, and have it run an otherwise disabled speed test. This way we can develop with the load time in mind without worrying about individual system speed.

Michael G. Schwern
Owner

This small a feature isn't worth a 50ms jump in load time. That's a 30% increase BTW. Its value is further reduced in that there's no synergy with perl5i. It's just a CPAN module we're loading by default, same as anyone else can.

I hear you about the load time problem. It's possible we could calibrate it in some way. Like check how long loading a certain set of modules takes, (do it a few times to make sure it's disk cached) and then check we're not taking much longer than that. Use the list of modules we currently use as a baseline.

Chad Granum
Collaborator
ben hengst
Collaborator

suggesting we close this one?

Michael G. Schwern
Owner

I'd like to keep it open. Its a good idea, but the load time was excessive.

I tried it again and the load time has been greatly reduced, something like 10ms on top of perl5i::2. This is within the realm of what is acceptable. I don't know what the runtime costs are, worth investigating.

This is something which can be pushed forward again.

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.