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.
Hide
Stuart Sierra added a comment -

I think I'm starting to get a handle on this. There's a lot going on here, and it's been more than two years since I wrote most of this code, so bear with me.

At track.clj line 78 we compute the dependency order for unloading namespaces based on the dependency graph (the local deps) as it existed before the most recent set of changes. I believe that this is correct, or at least the correct intention. If we change a file such that its dependencies are different, the order in which we unload namespaces should reflect the namespaces in memory, as they were before we changed the file.

When using c.t.n.dir/scan to detect and reload changed files this works correctly, at least most of the time.

When adding files to a new tracker for the first time, the old dependency graph is empty, so the unload order is undefined. This is arguably incorrect but effectively meaningless, since those namespaces have not yet been loaded.

In release 0.2.6, there was a bad commit which mistakenly changed c.t.n.dir/scan and scan-all to remove the files which have changed before adding them again. As a result, the dependencies of changed files were removed from the tracker's dependency graph before the unload order could be calculated, so unload order was always undefined. I have reverted that change — thanks for drawing my attention to it!

Now the unload order should be "correct" after scan or scan-all, except for the first time files are added to the tracker, when unload order is undefined.

The tracker doesn't currently have a way to distinguish between changed files which need unload+load and new files which only need load. Even if there were a way to distinguish between new and changed files, we can't be certain that a namespace has not been loaded by other means (e.g. require at the REPL) so it's safest to unload everything before reloading.

In general, unload order shouldn't matter at all. Clojure doesn't care when namespaces are removed: the Vars are still referenced from wherever they are used.

Show
Stuart Sierra added a comment - I think I'm starting to get a handle on this. There's a lot going on here, and it's been more than two years since I wrote most of this code, so bear with me. At track.clj line 78 we compute the dependency order for unloading namespaces based on the dependency graph (the local deps) as it existed before the most recent set of changes. I believe that this is correct, or at least the correct intention. If we change a file such that its dependencies are different, the order in which we unload namespaces should reflect the namespaces in memory, as they were before we changed the file. When using c.t.n.dir/scan to detect and reload changed files this works correctly, at least most of the time. When adding files to a new tracker for the first time, the old dependency graph is empty, so the unload order is undefined. This is arguably incorrect but effectively meaningless, since those namespaces have not yet been loaded. In release 0.2.6, there was a bad commit which mistakenly changed c.t.n.dir/scan and scan-all to remove the files which have changed before adding them again. As a result, the dependencies of changed files were removed from the tracker's dependency graph before the unload order could be calculated, so unload order was always undefined. I have reverted that change — thanks for drawing my attention to it! Now the unload order should be "correct" after scan or scan-all, except for the first time files are added to the tracker, when unload order is undefined. The tracker doesn't currently have a way to distinguish between changed files which need unload+load and new files which only need load. Even if there were a way to distinguish between new and changed files, we can't be certain that a namespace has not been loaded by other means (e.g. require at the REPL) so it's safest to unload everything before reloading. In general, unload order shouldn't matter at all. Clojure doesn't care when namespaces are removed: the Vars are still referenced from wherever they are used.
Hide
Andy Fingerhut added a comment -

OK, makes sense. Is it already documented anywhere that the unload order is independent of the dependencies after scan-all (and perhaps other calls)? I can create a separate ticket for that if you think it is worth adding such documentation.

It sounds like for the application I had in mind, the reverse of the load order is always defined, and is a correct unload order. You are welcome to close this ticket.

Show
Andy Fingerhut added a comment - OK, makes sense. Is it already documented anywhere that the unload order is independent of the dependencies after scan-all (and perhaps other calls)? I can create a separate ticket for that if you think it is worth adding such documentation. It sounds like for the application I had in mind, the reverse of the load order is always defined, and is a correct unload order. You are welcome to close this ticket.
Hide
Stuart Sierra added a comment -

After release 0.2.7, which fixed the regression in 0.2.6, :unload order should be correct in all cases except the first time files are added to a new tracker, even with scan-all. I would appreciate it if you can confirm this in your application.

It may be possible to fix the new-tracker case too. For example, commit c0b6b93d, currently on the branch TNS-20-fix-initial-unload-order. This works for the tests in your patch.

I'm not sure if the same change is needed at track.clj line 104 as well.

Show
Stuart Sierra added a comment - After release 0.2.7, which fixed the regression in 0.2.6, :unload order should be correct in all cases except the first time files are added to a new tracker, even with scan-all. I would appreciate it if you can confirm this in your application. It may be possible to fix the new-tracker case too. For example, commit c0b6b93d, currently on the branch TNS-20-fix-initial-unload-order. This works for the tests in your patch. I'm not sure if the same change is needed at track.clj line 104 as well.
Hide
Andy Fingerhut added a comment -

My application right now is very simple compared to the component workflow – simply use dir/scan-all to get the namespaces and their dependencies, and then print them in a particular order consistent with a correct unload order. I'm sorry, but I don't have the interest right now in testing whether the unload order is correct after doing additional operations on the tracker, since it isn't needed in my application.

I do have a sanity check in my application that confirms the load order is consistent with the dependencies in my application, and will file a bug if I ever see that trigger anything, but I don't expect there is a bug there.

I have switched to using the reverse of the load order in my application for what I believe should always be a correct unload order.

I may look into the branch change you made, but it may fall off my radar unless my needs change.

Show
Andy Fingerhut added a comment - My application right now is very simple compared to the component workflow – simply use dir/scan-all to get the namespaces and their dependencies, and then print them in a particular order consistent with a correct unload order. I'm sorry, but I don't have the interest right now in testing whether the unload order is correct after doing additional operations on the tracker, since it isn't needed in my application. I do have a sanity check in my application that confirms the load order is consistent with the dependencies in my application, and will file a bug if I ever see that trigger anything, but I don't expect there is a bug there. I have switched to using the reverse of the load order in my application for what I believe should always be a correct unload order. I may look into the branch change you made, but it may fall off my radar unless my needs change.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated: