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

BREAKING: Many fixes; all leaks fixed #762

Merged
merged 12 commits into from Apr 22, 2016
Merged

BREAKING: Many fixes; all leaks fixed #762

merged 12 commits into from Apr 22, 2016

Conversation

@feross
Copy link
Member

feross commented Apr 21, 2016

Added

  • client.listening property to signal whether TCP server is listening for incoming
    connections.
  • client.dhtPort property to show the actual chosen DHT port (parallel to client.torrentPort for the TCP torrent listening server)

Changed

  • Merged Swarm class into Torrent object. Properties on torrent.swarm (like
    torrent.swarm.wires) now exist on torrent (e.g. torrent.wires). Deleted a ton of code!
  • Deprecate: Do not use torrent.swarm anymore. Use torrent instead.
  • torrent.addPeer can no longer be called before the infoHash event has been
    emitted.
  • Remove torrent.on('listening') event. Use client.on('listening') instead.
  • Remove support from TCPPool for listening on multiple ports. This was not used by
    WebTorrent and just added complexity. There is now a single TCPPool instance for the
    whole WebTorrent client.
  • Deprecate: Do not use client.download() anymore. Use client.add() instead.
  • Only pass torrent.infoHash to the Chunk Store constructor, instead of the Torrent
    instance itself, to prevent accidental memory leaks of the Torrent object by the
    store. (Open an issue if you were using other properties. They can be re-added.)
  • Non-fatal errors with a single torrent will be emitted at torrent.on('error'). You
    should listen to this event. Previously, all torrent errors were also emitted on
    client.on('error') and handling torrent.on('error') was optional. The new design is
    better since now it is possible to distinguish between fatal client errors
    (client.on('error')) when the whole client becomes unusable versus recoverable errors
    where only a single torrent fails (torrent.on('error')) but the client can continue to
    be used. However, if there is no torrent.on('error') event, then the error will be
    forwarded to client.on('error'). This prevents crashing the client when the user
    only has a listener on the client, but it makes it impossible for them to determine
    a client error versus a torrent error.

Fixed

  • If client.get is passed a Torrent instance, it now only returns it if it is present
    in the client.
  • Errors creating a torrent with client.seed are now emitted on the returned torrent
    object instead of the client (unless there is no event listeners on
    torrent.on('error') as previously discussed). The torrent object is now also destroyed
    automatically for the user, as was probably expected.
  • Do not return existing torrent object when duplicate torrent is added. Fire an
    'error' event instead.
  • Memory leaks of Torrent object caused by many internal subclasses of WebTorrent,
    including RarityMap, TCPPool, WebConn, Server, File, Torrent.
  • client.ratio and torrent.ratio are now calculated as uploaded / received instead
    of uploaded / downloaded.

Closes #721
Closes #712
Closes #635
Closes #317
Closes #248

feross added 12 commits Apr 21, 2016
### Added

- `client.listening` property to signal whether TCP server is listening
for incoming
  connections.

### Changed

- Merged `Swarm` class into `Torrent` object. Properties on
`torrent.swarm` (like
  `torrent.swarm.wires`) now exist on `torrent` (e.g. `torrent.wires`).

- `torrent.addPeer` can no longer be called before the `infoHash` event
has been
  emitted.

- Remove `torrent.on('listening')` event. Use `client.on('listening')`
instead.

- Remove support from `TCPPool` for listening on multiple ports. This
was not used by
  WebTorrent and just added complexity. There is now a single `TCPPool`
instance for the
  whole WebTorrent client.

- Deprecate: Do not use `client.download()` anymore. Use `client.add()`
instead.

- Deprecate: Do not use `torrent.swarm` anymore. Use `torrent` instead.

### Fixed

- When there is a `torrent.on('error')` listener, don't also emit
  `client.on('error')`.

- Do not return existing torrent object when duplicate torrent is
added. Fire an
  `'error'` event instead.

- Memory leak of `Torrent` object caused by `RarityMap`

- Memory leak of `Torrent` object caused by `TCPPool`

- `client.ratio` and `torrent.ratio` are now calculated as `uploaded /
received` instead
  of `uploaded / downloaded`.
- Only pass `torrent.infoHash` to the Chunk Store constructor, instead
of the `Torrent`
  instance itself, to prevent accidental memory leaks of the `Torrent`
object by the
  store. (Open an issue if you were using other properties. They can be
re-added.)

- Non-fatal errors with a single torrent will be emitted at
`torrent.on('error')`. You
  should listen to this event. Previously, all torrent errors were also
emitted on
  `client.on('error')` and handling `torrent.on('error')` was optional.
This design is
  better since now it is possible to distinguish between fatal client
errors
  (`client.on('error')`) when the whole client becomes unusable versus
recoverable errors
  where only a single torrent fails (`torrent.on('error')`) but the
client can continue to
  be used. However, if there is no `torrent.on('error')` event, then
the error will be
  forwarded to `client.on('error')`. This prevents crashing the client
when the user
  only has a listener on the client, but it makes it impossible for
them to determine
  a client error versus a torrent error.

- Errors creating a torrent with `client.seed` are now emitted on the
returned `torrent`
  object instead of the client (unless there is no event listeners on
  `torrent.on('error')` as previously discussed). The torrent object is
now also destroyed
  automatically for the user, as was probably expected.

- If `client.get` is passed a `Torrent` instance, it now only returns
it if it is present
  in the client.
@feross feross changed the title BREAKING: Many fixes for desktop; all leaks fixed BREAKING: Many fixes; all leaks fixed Apr 21, 2016
var hat = require('hat')
var Swarm = require('../lib/swarm')
var test = require('tape')
// var hat = require('hat')

This comment has been minimized.

Copy link
@dcposch

dcposch Apr 21, 2016

Contributor

Might want to delete the commented out files. Or are you planning to port the tests to use torrent instead of swarm later?

This comment has been minimized.

Copy link
@feross

feross Apr 21, 2016

Author Member

Planning to port the tests later.

@dcposch

This comment has been minimized.

Copy link
Contributor

dcposch commented Apr 21, 2016

This is awesome.

Also when you exclude release notes and commented-out tests, you're mostly removing code.

https://twitter.com/henrikjoreteg/status/545424384954228736

@feross

This comment has been minimized.

Copy link
Member Author

feross commented Apr 21, 2016

Swarm tests are disabled for now, since there's no Swarm class anymore to test. It's part of Torrent. I'll see what testing is worth keeping and what is redundant in a future PR.

@feross

This comment has been minimized.

Copy link
Member Author

feross commented Apr 21, 2016

I want to get a PR ready for webtorrent-cli ready before merging this. Will release tomorrow.

@grunjol

This comment has been minimized.

Copy link
Contributor

grunjol commented Apr 21, 2016

I think this is the PR everybody was waiting for :D.

@feross

This comment has been minimized.

Copy link
Member Author

feross commented Apr 22, 2016

PR to webtorrent-cli wasn't so bad: webtorrent/webtorrent-cli#6

@feross feross merged commit c85dd3b into master Apr 22, 2016
4 checks passed
4 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@feross feross deleted the fixes-for-desktop branch Apr 22, 2016
@lock

This comment has been minimized.

Copy link

lock bot commented May 4, 2018

This thread has been automatically locked because it has not had recent activity. To discuss futher, please open a new issue.

@lock lock bot locked as resolved and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.