search

npm / npm-registry-client

http://npm.im/npm-registry-client
ISC License
264 stars 108 forks source link

Concurrency with npm.publish can cause crash #114

Closed jamietre closed 9 years ago

jamietre commented 9 years ago

When under load (generating & publishing hundreds of modules with concurrency, against Sinopia) npm.publish can crash, perhaps when trying to read a module before it's been published completely. 

undefined
^
SyntaxError: Unexpected token u
    at Object.parse (native)
    at CachingRegistryClient.putNext (..\node_modules\npm\node_modules\npm-registry-client\lib\publish.js:169:20)
    at ..\npm\node_modules\npm-registry-client\lib\publish.js:128:15
    at ..\npm\lib\cache\caching-client.js:52:8
    at f (..\npm\node_modules\once\once.js:17:25)
    at ..\npm\node_modules\npm-registry-client\lib\request.js:69:16
    at f (..\npm\node_modules\once\once.js:17:25)
    at CachingRegistryClient.<anonymous> (..\npm\node_modules\npm-registry-client\lib\request.js:258:12)
    at Request._callback (..\npm\node_modules\npm-registry-client\lib\request.js:171:14)
    at Request.self.callback (..\npm\node_modules\request\request.js:198:22)

The error is here, root.maintainers is undefined:

169   var maint = JSON.parse(JSON.stringify(root.maintainers))
170   root.versions[newVersion].maintainers = maint

It seems like it could be trivially fixed..

if (root.maintainers) {
   root.versions[newVersion].maintainers = JSON.parse(JSON.stringify(root.maintainers))
}

... and it definitely solves my problem, however, I don't know if this is indicative of some other problem, or perhaps even an issue with Sinopia. Without understanding the code base too much, though, it looks like there could be legitimate scenarios where root.maintainers is not defined.

If there's seems to be a reason why root.maintainers isn't checked here I can try to create a reproducible scenario.

othiym23 commented 9 years ago

Since the maintainers field is created and maintained by the registry, defaulting its value on the client side is misleading at best and could inadvertently cause package metadata corruption at worst. It's a part of the contract with the server that existing packages will have that field set, so my inclination is to treat this as a bug in sinopia, and not the client.

jamietre commented 9 years ago

Looked at sinopia a bit - there are definitely some issues with concurrency (that is, no resource locking during package creation process) so seems likely a problem at that end. Thanks for the feedback.