race condition between 'data' and 'accept' events breaks pushover #16

Open
jfhbrook opened this Issue Nov 28, 2012 · 5 comments

Projects

None yet

4 participants

@jfhbrook

Sup SubbRubb,

I'm working on a pushover-based git server with some friends (pushover plus some extra routes), and we came across this error message:

piston info  PISTON listening on http://0.0.0.0:31337
piston info  -> GET /test/piston.git/info/refs?service=git-receive-pack
piston info  -> POST /test/piston.git/git-receive-pack
piston info  -> GET /test/piston.git/info/refs?service=git-receive-pack
piston info  -> POST /test/piston.git/git-receive-pack
piston ERR!  ☠☠☠  FLAGRANT SYSTEM ERROR ☠☠☠
piston ERR!  Cannot call method 'push' of undefined
piston ERR!  TypeError: Cannot call method 'push' of undefined
piston ERR!      at Service.ondata (/home/nathan/piston/node_modules/pushover/lib/service.js:36:18)
piston ERR!      at Service.EventEmitter.emit (events.js:126:20)
piston ERR!      at IncomingMessage.EventEmitter.emit (events.js:96:17)
piston ERR!      at IncomingMessage._emitData (http.js:359:10)
piston ERR!      at HTTPParser.parserOnBody [as onBody] (http.js:123:21)
piston ERR!      at Socket.socket.ondata (http.js:1704:22)
piston ERR!      at TCP.onread (net.js:403:27)
Forever detected script exited with code: 1

We did some investamagating, and it looks like what's happening is that this is getting called:

https://github.com/substack/pushover/blob/master/lib/service.js#L84

which sets buffered to undefined, and then a data event gets unexpectedly fired which hits:

https://github.com/substack/pushover/blob/master/lib/service.js#L33-L36

Any ideas as to why this is happening? fwiw it all seems to work on localhost-only test envs, it's only when pushover is on a remote (running 0.8.x, we checked).

/cc @nathan7

@nathan7

It is perhaps worth noting that no .on('push') callback event listener is provided, triggering the automatic accept behaviour.

@nathan7

This doesn't seem to change anything:

.on('push', function(push) {
  process.nextTick(function() {
    push.accept();
  });
});

This, however, works:

.on('push', function(push) {
  setTimeout(function() {
    push.accept();
  }, 100);
});

Seems like a race condition.

@substack
Owner

I ran into possibly the same bug today in production and pushed a fix in 1.1.1.

@nathan7

Sweet! Thanks.

@martindale

What's the status on this? This seems similar to #30.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment