add closed? method on Stream object to inspect the object's open/close state #535

Merged
merged 1 commit into from Dec 13, 2012

Conversation

Projects
None yet
5 participants
Contributor

yeban commented Jun 24, 2012

So I have a list of streaming connections accumulated in a connections variable.

get '/' do
  stream(:keep_open) { |out| connections << out }
end

Over time the list of connections can get huge, and I would like to trim the dead ones:

connections.select(&:closed?).each(&:close)

Inspecting @closed on a stream object doesn't seem like the best way to go about it.

@yeban yeban add closed? method on Stream object to inspect the object's open/clos…
…e state

Signed-off-by: Anurag Priyam <anurag08priyam@gmail.com>
cc8502f
Owner

rkh commented Jun 24, 2012

sinatra-contrib adds closed?. Not sure if and what we should move to sinatra proper.

This pull request passes (merged cc8502f into 8adecc0).

Contributor

yeban commented Jun 24, 2012

To me, Stream#closed? seems too basic a functionality to be residing in a separate gem. In fact, I was so convinced it is there that I used it in my app without even consulting the docs or the source, only to get a NoMethodError :P.

pirj commented Aug 12, 2012

sinatra-contrib adds unexpected behavior :

sinatra-contrib-1.3.1/lib/sinatra/streaming.rb:119:in `<<': not opened for writing (IOError)

This is not the case if sinatra/streaming is not used.
Besides that it works like a charm.

rkh referenced this pull request in sinatra/sinatra-contrib Aug 12, 2012

Closed

rethink sinatra-streaming, maybe #60

troyk commented Aug 23, 2012

I was cruising the code to figure out how to deal with closed connections and googled a bit to see what others were doing and landed here. Not saying "closed?" isn't needed, but the use case listed above doesn't make any sense based on the code:

connections.select(&:closed?).each(&:close)

doesn't do anything but return

def close
  return if @closed
  @closed = true
  @scheduler.schedule { @callbacks.each { |c| c.call }}
end

It seems the proper thing to do is to set a callback to delete "out" from connections which will be triggered by the close method, but I'm unfamiliar with how long the scheduler will take to invoke the callbacks and if another event (or thread) is iterating the connections to send data it probably makes sense to do something like below, as Stream::<< doesn't check if the connection is closed. I'm assuming writing to a closed connection is going to throw something somewhere:

connections.each do |out|
  out << some_data unless out.closed?
end

And if that makes sense, adding closed? seems like a great idea.

Contributor

yeban commented Aug 23, 2012

@troyk writes:

Not saying "closed?" isn't needed, but the use case listed above doesn't make any sense based on the code:

connections.select(&:closed?).each(&:close)

My bad. I have no idea what I was thinking when I wrote that two months ago. I probably wanted to say:

connections.reject!(&:closed?)

@rkh rkh added a commit that referenced this pull request Dec 13, 2012

@rkh rkh Merge pull request #535 from yeban/stream_closed
add closed? method on Stream object to inspect the object's open/close state
d2e8563

@rkh rkh merged commit d2e8563 into sinatra:master Dec 13, 2012

Owner

rkh commented Dec 13, 2012

@yeban Could you add some docs?

Contributor

yeban commented Dec 14, 2012

@rkh Sure. Over the weekend.

yeban referenced this pull request Dec 22, 2012

Merged

Update stream documentation #599

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