Skip to content
This repository has been archived by the owner on Dec 18, 2017. It is now read-only.

Sync the breadcrumbs writer #1775

Merged
merged 1 commit into from
May 6, 2015
Merged

Conversation

victorhurdugaci
Copy link
Contributor

Fixes: #1733 and #1771

The root cause is this change:

context.DependencyWalker.Walk(project.Name, project.Version, target.TargetFramework);
. We write the breadcrumbs during the dependency graph walking. That code was called only once before and it happened before breadcrumb writing.

Now that is it called multiple times, it can happen to run at the same time as the breadcrumb writing that that's running on a different thread.

This change will allow Add to be called at anytime and only throw if you try to add a new breadcrumb (one that didn't exists when write was called).

Also, the change includes a lock for multi-process breadcrumb writing. It locks the breadcrumb file and doesn't allow other processes to access it while writing.

Please review: @davidfowl @pranavkm

@ghost ghost added the cla-already-signed label May 4, 2015
@Eilon
Copy link
Member

Eilon commented May 4, 2015

Two things we discussed that require follow-up:

  1. Why is the dependency tree being walked twice? Is that a bug? Or on purpose? Are the results the same? Cache?
  2. Is it allowed/supported for user code to load NuGet packages while the app is running? If so, we should theoretically breadcrumb those, even though it might be technically challenging.

@victorhurdugaci
Copy link
Contributor Author

@davidfowl the two questions above are for you. @Eilon and I discussed offline

@davidfowl
Copy link
Member

Hah! My change caused those problem 😊

@Eilon
Copy link
Member

Eilon commented May 5, 2015

@davidfowl you owe @victorhurdugaci $10 plus a dozen donuts.

@victorhurdugaci
Copy link
Contributor Author

@davidfowl we can make than just a dozen donuts if you answer the Eilon's two questions 😄

@davidfowl
Copy link
Member

I'm wondering if we can turn of breadcrumbing for compilation purposes or move where it is done so it doesn't happen more than once?

@victorhurdugaci
Copy link
Contributor Author

We can do that. It would be the same ignoring any breadcrumb add once writing has started. That leads to @Eilon 's second question: is it possible to load NuGet packages dynamically while the app is running?

@davidfowl
Copy link
Member

Ok here's what we need to do:

  • Make the servicing classes services that are passed into the NuGetDependencyResolver instead of being a singleton (a singleton may be passed in but don't assume it is one).
  • Make it so that a bool can be passed in that will disable ball bread crumbing

@victorhurdugaci
Copy link
Contributor Author

•Make it so that a bool can be passed in that will disable ball bread crumbing

Are you okay with simply ignoring adds after writing has started?

@davidfowl
Copy link
Member

You can absolutely call Assembly.Load or write your own loader that does something similar to what we do but that's not our problem

@davidfowl
Copy link
Member

Yes we actually have this in the wrong layer. Everyone uses this api to reason about the dependency graph. We really shouldn't be bread crumbing anything that wouldn't get loaded. As much, it should only ever be true when the ApplicationHost runs

@victorhurdugaci
Copy link
Contributor Author

Updated. No more adds after write

@@ -72,12 +77,20 @@ public void AddBreadcrumb(string packageId, SemanticVersion packageVersion)
{
return;
}

_breadcrumbsToWrite.Add(new BreadcrumbInfo()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove whitespaces

@davidfowl
Copy link
Member

Whats the deal with this?

@dnfclas
Copy link

dnfclas commented May 6, 2015

@victorhurdugaci, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

{
internal static class ConcurrencyUtilities
public static class ConcurrencyUtilities
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it is used by the package manager too. Please note that I moved the file in a different project

@davidfowl
Copy link
Member

:shipit:

…don't have problems sharing

- Once breadcrumb writing has started, ignore adds
@victorhurdugaci victorhurdugaci merged commit ed972d4 into dev May 6, 2015
@victorhurdugaci victorhurdugaci deleted the victorhu/breadcrumbs-sync branch May 6, 2015 17:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sharing violation on breadcrumbs files
6 participants