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

A Proof of concept patch that 'fixes' #67 #113

Closed
wants to merge 1 commit into from

Conversation

ciaranj
Copy link

@ciaranj ciaranj commented Sep 1, 2014

  • I made various attempts to get flock working properly on windows for sinopia.
    They all seem to result in the process ending un-expctedly :/
  • So in the short term this patch does seem to address a bunch of realworld issues people are seeing, however
    it also causes 3 new test breaks, so I doubt you want to actually merge it.

If I have time to cycle back to this problem I might look at re-jigging it
again

- I made various attempts to get flock working properly on windows for sinopia.
  They all seem to result in the process ending un-expctedly :/
- So in the short term this patch does seem to address a bunch of realworld issues people are seeing, however
  it also causes 3 new test breaks, so I doubt you want to actually merge it.

If I have time to cycle back to this problem I might look at re-jigging it
again
@rlidwka
Copy link
Owner

rlidwka commented Sep 1, 2014

fs.exists is almost always a bad idea because of a potential race condition.

Can you try a rename-unlink-rename instead?

fs.rename(file1, file2, function(err) {
    if (err) {
        // windows
        fs.unlink(file2, function(err) {
            fs.rename(file1, file2, function(err) {
                cb()
            })
        })
    } else {
        cb()
    }
})

@ciaranj
Copy link
Author

ciaranj commented Sep 1, 2014

Interesting, I've tried that, and the failures come back :/ I should note that if any race exists, it is within the code, I trigger it with a single request for a package that sinopia has never seen before. (no other active clients.)

Whilst applying your suggested fix, I noted: https://github.com/rlidwka/sinopia/blob/master/lib/local-fs.js#L59 should that have an 'else' keyword before the 'stream.emit('success')' statement [incidentally, that doesn't fix the problems :/]

rlidwka added a commit that referenced this pull request Sep 3, 2014
@rlidwka
Copy link
Owner

rlidwka commented Sep 3, 2014

Whilst applying your suggested fix, I noted

I think you just found why our travis build fails randomly. This is great. :)

I'll try to get a windows box to debug those issues.

@ciaranj
Copy link
Author

ciaranj commented Sep 3, 2014

sweet. drive-by-bug-fix ;)

@rlidwka
Copy link
Owner

rlidwka commented Nov 4, 2014

Fixed in a854d07 (sinopia@0.13.2), see #67 (comment) for details . Sorry for the delay.

@rlidwka rlidwka closed this Nov 4, 2014
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.

2 participants