Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use archiveUrl when specified. #1174

Closed
wants to merge 1 commit into from
Closed

Use archiveUrl when specified. #1174

wants to merge 1 commit into from

Conversation

isoos
Copy link
Collaborator

@isoos isoos commented Dec 9, 2022

@isoos isoos requested review from jonasfj and sigurdm December 9, 2022 09:08
@sigurdm
Copy link
Contributor

sigurdm commented Dec 9, 2022

I also think we should not implement Pana should use dart pub to download and extract packages. #1170, as it also relies on the potentially cached version list.

Not sure this is true if I read it right. pub doesn't rely on the cached version listings - only as a heuristic to speed up requesting version listings for transitive dependencies.

Copy link
Contributor

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

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

LGTM

@sigurdm
Copy link
Contributor

sigurdm commented Dec 9, 2022

Not sure this is true if I read it right. pub doesn't rely on the cached version listings - only as a heuristic to speed up requesting version listings for transitive dependencies.

Ah you were talking about the cached version listings on pub.dev - just forget what I wrote then.

@jonasfj
Copy link
Member

jonasfj commented Dec 12, 2022

Not sure this is true if I read it right. pub doesn't rely on the cached version listings - only as a heuristic to speed up requesting version listings for transitive dependencies.

Ah you were talking about the cached version listings on pub.dev - just forget what I wrote then.

@isoos I don't understand this concern. The dart pub client will rely on the same APIs as the downloadPackage method uses.

Do you mean that dart pub might reuse something already downloaded in PUB_CACHE?
I don't really see what is wrong with that?
Certainly not in the future where we run each package in it's own VM. It's unlikely that newer or older versions of the same package will depend on the same package, actually I'm not sure how that would happen.

But regardless of whether we could reuse something from PUB_CACHE; I don't see a problem with that. The PUB_CACHE is intended to be immutable, let's trust it to be. Or let's introduce something like dart pub cache purge <package>, if we really want it -- I don't think it's necessary.

@isoos
Copy link
Collaborator Author

isoos commented Dec 12, 2022

@isoos I don't understand this concern. The dart pub client will rely on the same APIs as the downloadPackage method uses.

Exactly, and that is causing analysis failures right now. This PR allows us to feed the archiveUrl to pana without going through the cached API response.

The root cause of this issue is the cache: we receive a new version, but the cache may be stale for 10 minutes (either because we have split traffic or failed to purge it), during which any analysis that uses the API will fail for the new version.

@sigurdm
Copy link
Contributor

sigurdm commented Dec 12, 2022

Could we make some retry-logic in pana (or perhaps pub-worker) to take these possible 10 minutes into account?

  • pub get
    • (wait 1 minute)
  • pub get
    • (wait 2 minutes)
  • pub get
    • (wait 3 minutes)
  • pub get
    • (wait 4 minutes)
  • pub get
    • give up.

...or something even more clever

@isoos
Copy link
Collaborator Author

isoos commented Dec 12, 2022

I don't think pub get should have any minute+ retry. A few seconds may be okay for transient issues, but the process shouldn't hang for minutes just because there may be a cache invalidation down the line...

Coming from that angle, maybe this change is not needed either, instead, we should have a better retry logic on pub.dev, e.g. invalidating the cache if the first attempt of a latest version fails. (And do the second attempt after the cache has been invalidated.)

@isoos
Copy link
Collaborator Author

isoos commented Dec 13, 2022

Closing this as we will land the cache purge instead.

@isoos isoos closed this Dec 13, 2022
@isoos isoos deleted the url branch December 13, 2022 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants