Fix bug with concurrent renaming #1077

gregberge · 2014-01-27T22:58:53Z

We stock the renaming promise in cache to ensure that there is not concurrent renaming.

It solves the problem, i have added a test to ensure that it's corrected.

The only remaining problem can be when we run bower in parallel but it's actually not supported (i think).

Fix #933

We stock the renaming promise in cache to ensure that there is not concurrent renaming.

sindresorhus · 2014-01-28T08:59:57Z

The only remaining problem can be when we run bower in parallel but it's actually not supported (i think).

It almost does and the intention I guess it that it fully should: #596

gregberge · 2014-01-28T09:17:43Z

I don't see how to handle multiple rename over several processes right now. But however I think that first we must fix the actual problem and then if we have a problem we open another issue.

vvo · 2014-01-28T09:32:00Z

👍 concurrent rename in the same process is something that is hitting a lot more of users than
parallel installs (30+ participants versus 9).

@sindresorhus Do you think this could be merged soon?

wibblymat · 2014-02-10T13:38:38Z

Did #1076 fix this?

gregberge · 2014-02-10T13:44:32Z

No, #1076 decreases chances of problem described in #933.
But this issue fixes #933 by preventing two concurrent renaming of folders.

gregberge · 2014-02-10T14:04:46Z

@wibblymat you can merge it safely, I am using these patched version of bower and there is no problem (#933) since two weeks now.

wibblymat · 2014-02-13T15:00:31Z

lib/core/ResolveCache.js

+        else {
+            var defer = Q.defer();
+            defer.resolve();
+            checkExistingDirectory = defer.promise;


You should be able to replace these three lines with just checkExistingDirectory = new Q();

@wibblymat fixed in 8016bd4

evillemez · 2014-02-24T16:11:58Z

Any news on if/when this will get merged?

gregberge · 2014-02-24T16:27:32Z

+1

mhaligowski · 2014-03-01T11:14:41Z

@wibblymat when this is going to be merged? AFAIK it affects many people right now.

MathiasTim · 2014-03-06T11:57:08Z

+1 for a merge, having the same problem here with our jenkis build process

Paul-PSDigital · 2014-03-11T17:39:34Z

hear hear!

vvo · 2014-03-11T22:21:34Z

update: After some testing on my CI system using https://github.com/neoziro/bower/tree/v1.2.8-patched it seems to resolve the problem completely. Provided it doesn't have any other side-effects, this seems good to me from a functional test standpoint (not vouching for formatting, unit tests, etc) as my build passes 100 times in a row instead of failing 4/5.

unbreakable build driven development, merge ! :-)

lukebarton · 2014-03-12T10:03:17Z

Please can we get this through the appropriate release channel asap?

PowerKiKi · 2014-03-12T10:05:26Z

Our Travis CI builds are (almost) systematically broken because of that. A release would be greatly appreciated.

gregberge · 2014-03-12T11:30:31Z

I will close the PR since there is no response from maintainers.

lukebarton · 2014-03-12T11:32:42Z

You are joking right?
On 12 Mar 2014 11:30, "Greg Bergé" notifications@github.com wrote:

I will close the PR since there is no response from maintainers.

Reply to this email directly or view it on GitHubhttps://github.com//pull/1077#issuecomment-37398402
.

PowerKiKi · 2014-03-12T12:28:56Z

Please consider re-opening this PR. The issue affects several people and all feedback on this PR are positive. I don't see any valid reason to close it without any feedback from core developers.

This is something we desperately need, unless proven otherwise. And so far nothing against this PR was mentioned.

benschwarz · 2014-03-12T12:58:24Z

We're not done here yet. As a team we need to figure out the right thing to do.

evillemez · 2014-03-12T13:10:37Z

@neoziro Please please pretty please re-open this. Closing the PR won't make the problem go away - and this is a serious issue that is affecting quite a few people. @wibblymat What will it take to get this merged once it's reopened?

We are also using the neoziro/v1.2.8-patched branch at the moment because it's the only way we can seem to get around install fails, which is breaking both our CI testing and deployments.

If there's something wrong with the PR, could the maintainers please comment and give some direction?

wildlyinaccurate · 2014-03-14T10:16:46Z

I know "+1" comments are frowned upon but I just wanted to add my voice so it's clear how many people are affected.

vvo · 2014-03-14T10:24:05Z

we should remember that the number of +1 only shows the people who took the
time to:

search for the bug (can take hours)
find github
maybe create an account
add comment

And then there are the other people that are only facing the bug and did
not showed up here.

How many hours lost?

On 14 March 2014 11:17, Joseph Wynn notifications@github.com wrote:

I know "+1" comments are frowned upon but I just wanted to add my voice so
it's clear how many people are affected.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1077#issuecomment-37633157
.

Vincent Voyer
06 13 92 69 96

mistergaskill · 2014-03-14T17:25:38Z

This issue affects my team as well. We have been using this branch for a month as bower 1.2 is unusable for us without it.

lukebarton · 2014-03-14T20:08:52Z

Also remember that this PR is not the only report of this bug. I have seen
at least two or three tickets reporting this same bug, but none had gone as
far to provide a solution.
On 14 Mar 2014 10:24, "Vincent Voyer" notifications@github.com wrote:

we should remember that the number of +1 only shows the people who took
the
time to:

search for the bug (can take hours)

find github

maybe create an account

add comment

And then there are the other people that are only facing the bug and did
not showed up here.

How many hours lost?

On 14 March 2014 11:17, Joseph Wynn notifications@github.com wrote:

I know "+1" comments are frowned upon but I just wanted to add my voice
so
it's clear how many people are affected.

Reply to this email directly or view it on GitHub<
https://github.com/bower/bower/pull/1077#issuecomment-37633157>
.

Vincent Voyer
06 13 92 69 96

Reply to this email directly or view it on GitHubhttps://github.com//pull/1077#issuecomment-37633762
.

evillemez · 2014-03-14T22:15:02Z

@benschwarz You reopened the PR - do you know of anything that is preventing it from being merged?

quentindemetz · 2014-03-20T18:18:29Z

I've been using neoziro's fork for the past month, which completely resolved our previous deployment issues. Stock bower would randomly fail about 50% of the time.

pyromaniac · 2014-03-24T06:41:18Z

👍, we are waiting for this fix so much

gregberge · 2014-03-24T16:16:06Z

This issue is a joke.

vvo · 2014-03-24T16:24:50Z

Many users reported that this pull request has no critical side effects and is far more stable
in continuous environments than the current bower.

We understand that bower team may find a smarter/greater way to fix this bug in the future
and have it refactored like you like.

But in the meantime, the best action would have been to
merge this and open another issue where the bower team would say "Hey, we fixed the critical bug with .. but a better approach would be to do .."

Incremental strategy in the particular case of a critical bug (a bug that is impacting many users) seems more interesting than "wait, we will find a perfect way to do it, maybe later..".

pyromaniac · 2014-03-25T00:23:48Z

@neoziro, what do you mean?
Also, could you please update your patch with current master to simplify merging work?

gregberge · 2014-03-25T06:45:25Z

@pyromaniac no sorry, the bower team don't care about this problem, I gave time to fix it but now it's over. If they have treated the reals problems instead of adding the support of SVN, there would't have any merging problem.

There are maybe a thousand of people impacted by this, the quick fix is right there, it didn't completely solve the problem, but it's a good quick fix. They don't care about community and production problems linked to bower.

ppawel · 2014-03-26T14:44:43Z

Will look into switching to component or nuGet or something else from the next project because of this issue. Bower apparently cannot be trusted as a critical piece of project infrastructure...

xaka · 2014-04-04T08:20:03Z

bower team, hello hello? can we have your attention here as a community, please? @sindresorhus

desandro · 2014-04-04T11:58:44Z

Hi there! Sorry for the radio silence. We have a tremendous back log of issues we're trying to work our way through. We appreciate your patience.

@neoziro Thank you so much for your contribution. This is tremendous work. Don't give up on us!

@benschwarz @wibblymat This looks good to me. What concerns do we have around not merging? Do we have additional work that's coming down the pipeline that could resolve this?

benschwarz · 2014-04-05T04:41:29Z

Fix bug with concurrent renaming

DavidSouther · 2014-04-05T04:43:17Z

Hurrah! When can we expect a Bower release with these fixes?

sheerun · 2014-04-05T10:55:56Z

This PR doesn't solve NOENT problem, and it doesn't use write lock, so it:

Only solves ENOTEMPTY problem in one-process environment
Anything can happen between cache read and write

See my long comment in #933 (comment)

I think this should be reverted and #1211 merged.

xzyfer · 2014-04-06T03:41:24Z

FWIW I agree with @sheerun. This patch tackles a symptom rather than the root cause. We've been using this patch internally and continued to experience bower concurrency issues.

IMHO #1211 appears to be the more sensible approach.

…aming" This reverts commit cd541c7, reversing changes made to 3074711.

paulirish · 2014-04-07T00:49:27Z

Ben is about to back out 1077 and land @sheerun's #1211.

Thanks all for jumping in here and on 933. Appreciate it.

Fix concurrent renaming.

3ecc389

We stock the renaming promise in cache to ensure that there is not concurrent renaming.

gregberge mentioned this pull request Jan 27, 2014

random ENOTEMPTY in Travis run #933

Closed

Fix removing directory.

d130c73

gregberge mentioned this pull request Feb 10, 2014

Fix reading versions from cache directory. #1076

Merged

wibblymat reviewed Feb 13, 2014
View reviewed changes

Use new Q() shortcut.

8016bd4 8000

jrf0110 mentioned this pull request Mar 4, 2014

IE8 Is broke as hell goodybag/cater-api-server#706

Merged

mhaligowski mentioned this pull request Mar 4, 2014

ENOENT after 1.2.8 update #991

Closed

timelf123 mentioned this pull request Mar 5, 2014

EPERM error when installing Angulartics with Bower on Windows angulartics/angulartics#102

Closed

gregberge closed this Mar 12, 2014

benschwarz reopened this Mar 12, 2014

benschwarz added a commit that referenced this pull request Apr 5, 2014

Merge pull request #1077 from neoziro/fix-concurrent-renaming

cd541c7

Fix bug with concurrent renaming

benschwarz merged commit cd541c7 into bower:master Apr 5, 2014

benschwarz added a commit to benschwarz/bower that referenced this pull request Apr 6, 2014

Revert "Merge pull request bower#1077 from neoziro/fix-concurrent-ren…

20aca45

…aming" This reverts commit cd541c7, reversing changes made to 3074711.

benschwarz added a commit that referenced this pull request Apr 7, 2014

Update changelog with #1211, remove #1077

bdaa359

Uh oh!

Fix bug with concurrent renaming #1077

Fix bug with concurrent renaming #1077

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!