baconjs / bacon.js

Functional reactive programming library for TypeScript and JavaScript
https://baconjs.github.io
MIT License
6.47k stars 330 forks source link

Streams dispose logic #481

Closed gyzerok closed 9 years ago

gyzerok commented 9 years ago

For example i'm using bacon with socket.io like this:

var conns = Bacon.fromBinder(function (sink) {
    io.on('connect', sink);
});

Suppose there is no other streams referring conns. What would happen if now clear conns?

conns = null;

Would all the resources be correctly cleared? And what about anonymous streams? How to determine their lifetime? For example:

Bacon.combineAsArray(conns, property)
      .onValues(function (socket, property) {
             console.log(socket, property);
      });
raimohanska commented 9 years ago

How would this possibly work? If io.on gets called and io.off never gets called, if surely will leak.

But if you do

var cons=Bacon.fromEventTarget(io, "connect")

you'll get a properly behaving stream. Resource allocation and cleanup will be based on whether your stream has subscribers or not.

gyzerok commented 9 years ago

@raimohanska, and what about anonymous streams?

raimohanska commented 9 years ago

There's nothing special about them.

gyzerok commented 9 years ago

@raimohanska mb it's obvious for you, but not for me.

When following anonymous stream would be disposed?

Bacon.combineAsArray(conns, property)
      .onValues(function (socket, property) {
             console.log(socket, property);
      });
gyzerok commented 9 years ago

@raimohanska and when i'm trying to declare conns in your way i'm getting an error:

var conns = Bacon.fromEventTarget(io, 'connect');
this.engine.on('connection', this.onconnection.bind(this));
TypeError: Object connect has no method 'on'
raimohanska commented 9 years ago

Endless stream with a subscriber never gets disposed. Add a takeUntil or somethink else that defines a meaningful scope. Please read the docs too. Unfortunately I don't have too much time for personal support.

raimohanska commented 9 years ago

This is not a support forum. I kindly ask you to use Google Groups or Stack Overflow.

raimohanska commented 9 years ago

Error reports belong here of course. Sorry if I hurt your feelings. Didn't mean to. I'm kinda having a bad day...

gyzerok commented 9 years ago

Ok, no problem. No more questions here, I understood. But I kindly ask you to check if it's a bug or not.

var conns = Bacon.fromBinder(function (sink) {
    io.on('connect', sink);
});

Works fine.

Your solution:

var conns = Bacon.fromEventTarget(io, 'connect');
this.engine.on('connection', this.onconnection.bind(this));
TypeError: Object connect has no method 'on'
raimohanska commented 9 years ago

Do you have a runnable example?

raimohanska commented 9 years ago

I assume you have the latest baconjs 0.7.37 or so?

gyzerok commented 9 years ago

Here you can find working example. Yes, i'm using the latest version. You can check it in package.json

raimohanska commented 9 years ago

The hint's in the stack trace, mate.

/Users/juha/code/bacon.js/testing/bacon-bug-example/node_modules/socket.io/lib/index.js:300
  this.engine.on('connection', this.onconnection.bind(this));
              ^
TypeError: Object connection has no method 'on'
    at Server.bind (/Users/juha/code/bacon.js/testing/bacon-bug-example/node_modules/socket.io/lib/index.js:300:15)
    at /Users/juha/code/bacon.js/testing/bacon-bug-example/node_modules/baconjs/dist/Bacon.js:85:11

Coincidentally the io object has a bind method which is one of the method names that fromEventTarget tries. I think we could improve the logic for detecting which method pair (bind/unbind, on/off, addListener/removeListener ..) should be used. In this case, we could rule out bind/unbind because there's no unbind. So indeed, there's room for improvement!

Wanna try and fix? I gotta sleep :)

raimohanska commented 9 years ago

Just for your reference, a working fromBinder solution could look like this.

var conns = Bacon.fromBinder(function (sink) {
  io.on('connect', sink);
  return function() {
    io.off('connect', sink);
  }
});
gyzerok commented 9 years ago

Not sure if I have enough skills to do it, but I would give it a try.

Can you tell me some other libraries I have to test my possible solution with? Its Backbone and ...

raimohanska commented 9 years ago

I'd rather have the relevant cases covered in automatic tests than test "manually" with different libraries. There's a bunch of tests for fromEventTarget in spec/BaconSpec.coffee.

raimohanska commented 9 years ago

Fixed along with #506