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.

Sign up for GitHub

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

Jump to bottom

Add WebSocket and SSE options #1272

Closed
wants to merge 7 commits into from
Closed

Add WebSocket and SSE options #1272

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fb6a756
Add WebSocket and SSE options
ohler55 Apr 28, 2018
42d535a
Don't use extend for push handler
ohler55 May 2, 2018
cb84901
Add client to on_message
ohler55 May 2, 2018
975acaa
client on all upgrade handler callbacks
ohler55 May 2, 2018
a742739
slight wording change
ohler55 May 2, 2018
0f047d3
Cleanup formatting
ohler55 May 9, 2018
7d15187
Add client env method
ohler55 Nov 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 19 additions & 0 deletions SPEC
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,25 @@ be implemented by the server.
fatal(message, &block)
<tt>rack.multipart.buffer_size</tt>:: An Integer hint to the multipart parser as to what chunk size to use for reads and writes.
<tt>rack.multipart.tempfile_factory</tt>:: An object responding to #call with two arguments, the filename and content_type given for the multipart form field, and returning an IO-like object that responds to #<< and optionally #rewind. This factory will be used to instantiate the tempfile for each multipart form file upload field, rather than the default class of Tempfile.

Servers that choose to support WebSockets and SSE connections should follow these additional environment specifications:
<tt>rack.upgrade?</tt>:: Is nil or missing if no connection upgrade is possible,
:websocket for a WebSocket upgrade, or :sse for an SSE upgrade.
<tt>rack.upgrade</tt>:: Is used to pass a handler instance or class back to the server for WebSocket and SSE connections.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, now that we support static handlers, this line should be altered?

Perhaps:

rack.upgrade:: Is used to pass a handler object back to the server for WebSocket and SSE connections.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, minor change.

The handler MAY implement any or all of the following callbacks:
on_open() # called when the connection is complete
on_message(message) # called when a WebSocket message is received by server. <tt>message</tt> will be UTF-8 encoded for text messages and Binary encoded for binary messages.
on_shutdown() # may be called before a connection is closed due to server shutdown.
on_close() # called when the connection is closed
on_drained # may be called when the number of pending writes drops to zero.
The object will be extended by the server to include:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break method caches. I'd prefer a solution that doesn't lean on runtime method extension. Maybe something like this:

class MyWSEObject
  def initialize io
    @io = io
  end

  def on_open
    @io.write("neat")
  end
end

app = lambda do |env|
  env['rack.upgrade'] = MyWSEObject.new(env['rack.wse.object'])
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

class MyWSEObject
  def on_open(io)
    io.write("neat")
  end
end

app = lambda do |env|
  env['rack.upgrade'] = MyWSEObject.new
end

would be simpler

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be fine too. 😄

Copy link
Member

@ioquatix ioquatix May 1, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the next question, is it a good idea to write into the request env to essentially send a response? To me, that's confusing. Perhaps at best it could be something like

if upgrade = env['rack.upgrade']
  return upgrade.call(MyWSEObject.new)
end

return [400, {}, []]

Perhaps the server could signal exactly what APIs are supported as in:

if upgrade = env['rack.upgrade.web_socket'] # or rack.upgrade.event_stream
  return upgrade.call(MyWSEObject.new)
end

return [400, {}, []]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH it doesn't really matter to me. Unfortunately though, env is really the only way that middleware can communicate. So writing to env is the only way that one middleware could indicate to the next "I'm responding by writing to a websocket". If the intention is that any "callable" (middleware / app) that sets the WSE object must not call anything else in the middleware chain, then that needs to be explicitly stated in SPEC.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ioquatix having the env['rack.upgrade'] respond to some kind of registration method would be a reasonable approach in my opinion. I feel a little uncomfortable having it provide a return value since it makes it less obvious what is being returned but my feelings are not very strong on that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ohler55 ya, I think passing the IO object to every callback (as @ioquatix suggested) is my preferred approach.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For C extensions, there's double the amount of objects per connection.

Why is this?

@tenderlove , I assume you know that the C server doesn't need an IO object, it uses a simple integer to keep track of IO.

This means that a C extension just needs to keep the callback object itself and add a secret variable to that object with the socket number.

This secret variable isn't possible in Ruby, but C extensions are allowed to create them. Since that variable is a Number, it's very small (it doesn't create an Object in the ObjectSpace realm and it's immutable).

This means a single object in C (the callback object). If we add the IO object, it's two objects - a 200% increase in object count.

In Ruby, we will have the callback handler, the raw IO object (both pre-existing) and an IO wrapper, a 150% increase in object count.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means a single object in C (the callback object). If we add the IO object, it's two objects - a 200% increase in object count.

What % increase is that in an actual app?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What % increase is that in an actual app?

That really depends on the app's use-case.

A micro services app running a WebSocket API will get hit harder than an HTTP app with the occasional WebSocket upload manager.

I think the issue is more pronounced because we are talking about long term objects.

Short term object count doesn't effect memory fragmentation as much as the fact that there's a long-term object blocking a memory "block" / "arena".

The more long-term objects are created (and eventually destroyed), the faster the fragmentation.

Having said that, even a 200% increase in object count is probably better than the solutions we currently employ when implementing WebSockets using hijack.

I won't make an issue out of it. I'll do what everyone thinks best, but I know in very practical terms (having coded both implementations in C) that extend is cheaper both in memory and in the number of method tree traversals required.

write(message) # writes to the WebSocket or SSE connection
close() # forces a close of the WebSocket or SSE connection
open?() # returns true if the connection is open, false otherwise
pending() # returns the number of pending writes or -1 if the connection is closed
This option should only be set it the environment options has a non-nil value for the <tt>rack.upgrade?</tt> option,
If the response status is 300 or higher, the server MUST ignore the <tt>rack.upgrade</tt> value (send the response without performing an upgrade).

The server or the application can store their own data in the
environment, too. The keys must contain at least one dot,
and should be prefixed uniquely. The prefix <tt>rack.</tt>
Expand Down