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

Race condition can result in torrent-stream preventing process exit #198

Closed
Trott opened this issue Sep 22, 2019 · 3 comments
Closed

Race condition can result in torrent-stream preventing process exit #198

Trott opened this issue Sep 22, 2019 · 3 comments

Comments

@Trott
Copy link

Trott commented Sep 22, 2019

If engine.destroy() is called before the 'ready' event for engine, then rechokeIntervalId is not set when clearInterval() is called, resulting in the setInterval() running endlessly and the app never exiting. It's not clear to me if this should be considered a bug in the user's program or a bug in torrent-stream but if a bug in the user's program, then it's a bug that exists in test/tracker.js. Run the test multiple times to make it hang, and more recent versions of Node.js seem to be more susceptible to the problem.

Refs:
nodejs/node#29504 (comment)

Is this a bug in the test/tracker.js, a bug in index.js, or a complete misunderstanding on my part?

@Trott
Copy link
Author

Trott commented Sep 22, 2019

I should have mentioned that the problematic test is this one:

test('peer should connect to an alternate tracker', function (t) {
  t.plan(5)
  var engine = null
  var server = new tracker.Server()
  server.once('listening', function () {
    t.ok(true, 'tracker should be listening')

    engine = torrents(torrent, { dht: false, trackers: ['http://127.0.0.1:54321/announce'] })
    engine.once('ready', function () {
      t.ok(true, 'should be ready')
    })
  })
  server.once('start', function (addr) {
    t.equal(addr, '127.0.0.1:6881')

    engine.destroy(function () {
      engine.remove(t.ok.bind(t, true, 'should be destroyed'))
    })
    server.close(t.ok.bind(t, true, 'tracker should be closed'))
  })
  server.listen(54321)
})

All the other ones have the detroy() call inside the ready() listener, but this one does not.

@Trott
Copy link
Author

Trott commented Sep 22, 2019

Here's a modification that seems to reliably demonstrate the problem. The test will pass but won't exit.

test('calling destroy() before \'ready\' should not hold event loop open', function (t) {
  t.plan(5)
  var engine = null
  var server = new tracker.Server()
  server.once('listening', function () {
    t.ok(true, 'tracker should be listening')

    engine = torrents(torrent, { dht: false, trackers: ['http://127.0.0.1:54321/announce'] })
    engine.destroy(function () {
      engine.remove(t.ok.bind(t, true, 'should be destroyed'))
    })
    engine.once('ready', function () {
      t.ok(true, 'should be ready')
    })
  })
  server.once('start', function (addr) {
    t.equal(addr, '127.0.0.1:6881')

    server.close(t.ok.bind(t, true, 'tracker should be closed'))
  })
  server.listen(54321)
})

Trott added a commit to Trott/torrent-stream that referenced this issue Sep 22, 2019
The 'ready' event sets up an interval timer that is cleared in
`destroy()`. However, in some situations, it is possible to for the
interval to be created after `destroy()` has run, resulting in an
interval timer that is never cleared, and thus an app that does not
exit when it is supposed to. This change fixes the issue and adds a
test for it.

Refs: mafintosh#198
@mafintosh
Copy link
Owner

Should be fixed in 1.2.0

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

No branches or pull requests

2 participants