Skip to content
This repository was archived by the owner on Dec 7, 2018. It is now read-only.

pass websockets through rack environment#17

Merged
tarcieri merged 10 commits intocelluloid:masterfrom
collin:master
Nov 20, 2012
Merged

pass websockets through rack environment#17
tarcieri merged 10 commits intocelluloid:masterfrom
collin:master

Conversation

@collin
Copy link
Copy Markdown
Contributor

@collin collin commented Nov 14, 2012

Made some changes so that a WebSocket request will go through the RackWorker.

Extracted some methods for remote addr/remote host and uri parts (path/query/fragment) to modules so the could be re-used on WebSocket requests.

Right now it just sticks the WebSocket instance in the rack env at env["async.connection"] Not really ideal, but then nothing about mixing websockets and rack together is ideal.

Also on the negative side is things will be weird if some middleware depend on [status, headers, body].

Having implemented it, I'm not 100% convinced it's a worthy idea. But when hacking around with things it can be nice to pretend Rack and WebSockets can live together in harmony.

@collin
Copy link
Copy Markdown
Contributor Author

collin commented Nov 14, 2012

saw #16 and rebased against master

@tarcieri
Copy link
Copy Markdown
Member

Sorry I haven't commented on this yet.

So this is... interesting. It'd be really neat if there were a standard for accommodating Websockets within Rack. Attempting to create one is likewise... interesting.

Not sure how I feel about this. Perhaps I should consult some outside opinions?

@ghost
Copy link
Copy Markdown

ghost commented Nov 16, 2012

good implementation.

i have a similar one, started cause i was unattentive and missed this pull request.
so just my two cents here:

  • perhaps closing socket when app responds with a status code higher than 300?
  • is not async.connection? too EM-ish? perhaps using rack.websocket instead?

@collin
Copy link
Copy Markdown
Contributor Author

collin commented Nov 16, 2012

@tarcieri yeah, I hear you. If you can get others talking and opining about it please do. Getting the websocket/rack situation resolved is incredibly important.

I found the reel websocket API a breath of fresh air. But the node community is kicking ass here, not because they have the best API, but the have an API that's easy to use without the rack baggage.

@slivu I think I like both of those ideas.

@zacksiri
Copy link
Copy Markdown

IMO right now websocket just seems like a pain in the ass, I've been looking into SSE lately, and it seems to be much nicer and a better fit for rack, perhaps we should look into that direction?

@ghost
Copy link
Copy Markdown

ghost commented Nov 16, 2012

@zacksiri, SSE are "supported out of the box" and has nothing to do with this pull request.

@ghost
Copy link
Copy Markdown

ghost commented Nov 16, 2012

Tony, imo it is a good start point.
I would encourage you to merge this, then we will can move forward with more Rack stuff.
Thank you.

@zacksiri
Copy link
Copy Markdown

What kind of baggage does rack have? @collin

@shtirlic
Copy link
Copy Markdown
Contributor

+1 to this maybe some git squash for commits

@collin
Copy link
Copy Markdown
Contributor Author

collin commented Nov 16, 2012

@zacksiri you have to return the [status,headers,body] tuple even though http allows for streamed responses. Any API to provide a socket-like object in rack has to hack around that basic rack assumption.

@tarcieri @slivu you guys think I should squash this down?

@tarcieri
Copy link
Copy Markdown
Member

@collin honestly I don't care. For a project like this I'd probably prefer not to squash

@collin
Copy link
Copy Markdown
Contributor Author

collin commented Nov 16, 2012

@tarcieri :) I will leave it alone.

@ghost
Copy link
Copy Markdown

ghost commented Nov 16, 2012

@collin, the important for me is to have it in master, no matter how :)

@tarcieri
Copy link
Copy Markdown
Member

@collin @Silvu Okay, how about this: I'm a fan of rack.websocket over async.connection. How about change that and we'll call it good? ;)

@collin
Copy link
Copy Markdown
Contributor Author

collin commented Nov 16, 2012

@tarcieri @slivu 💃

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we call just @app.call(env) here?

@collin
Copy link
Copy Markdown
Contributor Author

collin commented Nov 16, 2012

@shtirlic yes

@ghost
Copy link
Copy Markdown

ghost commented Nov 19, 2012

@tarcieri,

"change that and we'll call it good? ;)"

by calling it good do you mean merge it? :)

@tarcieri
Copy link
Copy Markdown
Member

@Silvu confirm

@ghost
Copy link
Copy Markdown

ghost commented Nov 20, 2012

@tarcieri, looks ok to me.

async stuff converted to rack.websocket, merged well with @shtirlic's updates and all specs passing.

please merge it to stop bothering you about this :)

thank you.

@tarcieri
Copy link
Copy Markdown
Member

Cool, let's give this a shot

tarcieri added a commit that referenced this pull request Nov 20, 2012
pass websockets through rack environment
@tarcieri tarcieri merged commit 667a233 into celluloid:master Nov 20, 2012
@tarcieri tarcieri mentioned this pull request Jan 5, 2013
kenichi pushed a commit to kenichi/reel that referenced this pull request Aug 4, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants