tools.namespace

tracker's unload order sometimes incorrect

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None

Description

The attached patch contains a new test namespace clojure.tools.namespace.load-unload-order-test that demonstrates the incorrect unload order, if you leave out the proposed fix in file track.clj

  1. tns-20-v1.patch
    23/Aug/14 3:02 PM
    6 kB
    Andy Fingerhut
  2. tns-20-v2.patch
    08/Sep/14 5:52 PM
    7 kB
    Andy Fingerhut

Activity

Hide
Andy Fingerhut added a comment -

Patch tns-20-v1.patch contains a test that demonstrates the bug, and contains a proposed fix. Please scrutinize the proposed fix carefully, as I haven't yet convinced myself 100% that it is the right fix.

Also adds one more dependency check in dependency_test.clj that I discovered while reviewing those tests, to model my dependency checking tests on.

Show
Andy Fingerhut added a comment - Patch tns-20-v1.patch contains a test that demonstrates the bug, and contains a proposed fix. Please scrutinize the proposed fix carefully, as I haven't yet convinced myself 100% that it is the right fix. Also adds one more dependency check in dependency_test.clj that I discovered while reviewing those tests, to model my dependency checking tests on.
Hide
Andy Fingerhut added a comment -

I've got some code that checks that the load and unload orders in a tools.namespace tracker are consistent with its dependencies. It is similar to the checks done now in dependencies_test.clj, except it assumes that the dependencies in the tracker are correct and verifies the load and unload orders against those.

If you would be interested in a patch that added these checks to tools.namespace, perhaps along with a debug/consistency-check flag that when true causes these checks to be run every time a tracker is updated, let me know.

Show
Andy Fingerhut added a comment - I've got some code that checks that the load and unload orders in a tools.namespace tracker are consistent with its dependencies. It is similar to the checks done now in dependencies_test.clj, except it assumes that the dependencies in the tracker are correct and verifies the load and unload orders against those. If you would be interested in a patch that added these checks to tools.namespace, perhaps along with a debug/consistency-check flag that when true causes these checks to be run every time a tracker is updated, let me know.
Hide
Stuart Sierra added a comment -

Thanks for this, Andy. I will need to study it further to convince myself this is the correct behavior.

To help, can you describe what the observed problem would be from a user's point of view?

We cannot assume that the dependencies in the tracker are always correct. I have demonstrable cases where they are wrong. Usually that takes the form of nonexistent namespaces left in the dependency graph from files that were deleted or had an invalid ns declaration.

Show
Stuart Sierra added a comment - Thanks for this, Andy. I will need to study it further to convince myself this is the correct behavior. To help, can you describe what the observed problem would be from a user's point of view? We cannot assume that the dependencies in the tracker are always correct. I have demonstrable cases where they are wrong. Usually that takes the form of nonexistent namespaces left in the dependency graph from files that were deleted or had an invalid ns declaration.
Hide
Andy Fingerhut added a comment -

I have not used tools.namespace for the reload workflow that it was originally developed for, so I can't say with certainty what the effect on a user would be, but I can make some guesses that you can probably confirm more quickly than I can.

The test case in the patch is one where a few Clojure source files are added to a tracker using function c.t.n.dir/scan-all. No other changes are made to the tracker. At that point, the dependencies are completely correct, and the load order calculated from those dependencies honors them, but the unload order does not.

If that tracker were then used in a call to c.t.n.reload/track-reload, track-reload would call remove-lib on a library B before calling remove-lib on a library A, even though A requires or uses B.

I guess your question confuses me a bit. Do you believe tools.namespace should only create trackers that have load and unload orders that honor the dependencies? Or is that a wrong assumption I was making from reading the implementation?

Show
Andy Fingerhut added a comment - I have not used tools.namespace for the reload workflow that it was originally developed for, so I can't say with certainty what the effect on a user would be, but I can make some guesses that you can probably confirm more quickly than I can. The test case in the patch is one where a few Clojure source files are added to a tracker using function c.t.n.dir/scan-all. No other changes are made to the tracker. At that point, the dependencies are completely correct, and the load order calculated from those dependencies honors them, but the unload order does not. If that tracker were then used in a call to c.t.n.reload/track-reload, track-reload would call remove-lib on a library B before calling remove-lib on a library A, even though A requires or uses B. I guess your question confuses me a bit. Do you believe tools.namespace should only create trackers that have load and unload orders that honor the dependencies? Or is that a wrong assumption I was making from reading the implementation?
Hide
Andy Fingerhut added a comment -

Attaching slightly cleaned up patch tns-20-v2.patch. Identical to tns-20-v1.patch except it avoids copying a function into the new test namespace by adding a dependency on the test namespace where it is defined.

Also updates the name of a deftest I had copied but not renamed in v1 of the patch.

Show
Andy Fingerhut added a comment - Attaching slightly cleaned up patch tns-20-v2.patch. Identical to tns-20-v1.patch except it avoids copying a function into the new test namespace by adding a dependency on the test namespace where it is defined. Also updates the name of a deftest I had copied but not renamed in v1 of the patch.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated: