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

Add onDispose method #276

Closed
wants to merge 1 commit into from
Closed

Add onDispose method #276

wants to merge 1 commit into from

Conversation

philipnilsson
Copy link
Member

A convenient method for cleaning up resources allocated when creating an Observable. I use this all the time, figured it's a candidate for core. I'll complement with tests if we want to include it.

@raimohanska
Copy link
Contributor

It seems the name is misleading, as the method actually returns another "wrapper" observable. Right?

@raimohanska
Copy link
Contributor

If we were to implement an onDispose method that doesn't require you to wrap the observable, that would require some kind of a fork in the Dispatcher. And that might indeed be a more useful / general-purpose method. Might add nicely to the visualization/analysis tools discussed in #273.

@philipnilsson
Copy link
Member Author

Yeah, naming is not so good. How about withDispose?

We could do a "pulling" variant of this as you suggest in the style of
onValue, but I do think like that this works like e.g. doAction, I
personally don't want this to force a subscription. Why do you say that'd
be more useful?

On Thu, Oct 31, 2013 at 8:45 PM, Juha Paananen notifications@github.comwrote:

If we were to implement an onDispose method that doesn't require you to
wrap the observable, that would require some kind of a fork in the
Dispatcher. And that might indeed be a more useful / general-purpose
method. Might add nicely to the visualization/analysis tools discussed in
#273 #273.


Reply to this email directly or view it on GitHubhttps://github.com//pull/276#issuecomment-27520292
.

@raimohanska
Copy link
Contributor

@philipnilsson the "pulling" variant must not force subscription, of course. That would defeat the purpose. Instead it just just observe the lifecycle of the stream by plugging into the Dispatcher mechanism.

I'm also a bit concerned about the actual s.onDispose(f) semantics. The current implementation calls f each time the subscriber count of s reaches zero, which may be more that once. Especially, the call to f does not guarantee that the stream is inactive forever, as new subscribers may appear later. The only situation where the stream truly is "dead" forever is when an End event comes through.

Hence, this is not necessarily the mechanism you'd like to use to free up resources. I'm not sure what kind of resources you're talking about, but shouldn't they be reserved again if a new subscriber re-activates the stream? Maybe we should introduce a pair onActivation / onDeactivation instead for resource management instead.

@philipnilsson
Copy link
Member Author

Are you talking about the "spy" feature?

Yeah I never really use the fact that you can go to zero subscribers and up
again, but maybe there should be a pair.

On Fri, Nov 1, 2013 at 7:21 AM, Juha Paananen notifications@github.comwrote:

@philipnilsson https://github.com/philipnilsson the "pulling" variant
must not force subscription, of course. That would defeat the purpose.
Instead it just just observe the lifecycle of the stream by plugging into
the Dispatcher mechanism.

I'm also a bit concerned about the actual s.onDispose(f) semantics. The
current implementation calls f each time the subscriber count of sreaches zero, which may be more that once. Especially, the call to
f does not guarantee that the stream is inactive forever, as new
subscribers may appear later. The only situation where the stream truly is
"dead" forever is when an End event comes through.

Hence, this is not necessarily the mechanism you'd like to use to free up
resources. I'm not sure what kind of resources you're talking about, but
shouldn't they be reserved again if a new subscriber re-activates the
stream? Maybe we should introduce a pair onActivation / onDeactivationinstead for resource management instead.


Reply to this email directly or view it on GitHubhttps://github.com//pull/276#issuecomment-27549604
.

@raimohanska
Copy link
Contributor

Well yeah, these new suggested methods would probably be useful for debugging/analysis/visualization in combo with spy.

@raimohanska
Copy link
Contributor

@philipnilsson are you planning on taking this further? I'm considering closing this for now, unless there's gonna be activity.

@philipnilsson
Copy link
Member Author

Depends on if the new changes in .7 affects this. I could try to write a
function that lets you open close resources, though i suspect that would
have to touch the dispatcher. Good idea?

@raimohanska
Copy link
Contributor

Now that 0.7 is out, and new year 2013S and all, we might consider getting back to business with #273, i.e. allow observing the lifecycle and content of all streams without them knowing. Maybe something along the lines of

Bacon.spy (stream) ->
  stream.spy {
    onEvent: (event) -> console.log "event", event.toString()
    onActivate: -> console.log "stream active", stream
    onDeactivate: -> console.log "stream inactive", stream
  }

Where onEvent would be triggered for all events dispatched through the stream. The onActivate / onDeactivate methods would be called when the first subscriber is added to the stream and when the last subscriber is removed. I think this should suffice even for NSA.

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

Successfully merging this pull request may close these issues.

None yet

2 participants