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

permessage-deflate #7

Open
manuel-rubio opened this issue Oct 24, 2018 · 9 comments
Open

permessage-deflate #7

manuel-rubio opened this issue Oct 24, 2018 · 9 comments

Comments

@manuel-rubio
Copy link

Hello, looks like now (at least for Google Chrome) it's a required extension. I checked the code and I guess that's the same as x-webkit-deflate-frame (or should be), what do you think?

@yurrriq
Copy link
Member

yurrriq commented Oct 24, 2018

Thanks for the report! I'm not familiar, but that seems plausible to me. Do you think we should treat x-webkit-deflate-frame and permessage-deflate the same way then? Hopefully someone who knows more can help us out here.

@manuel-rubio
Copy link
Author

@yurrriq I've tried to add the "permessage-deflate" as "x-webkit-deflate-frame", but looks like the compression is made in a different way, it's not working and the bad news are this kind of extension is activated by default in browsers like Chrome. Anyway, cowboy works fine so, I had to move to cowboy instead.

@yurrriq
Copy link
Member

yurrriq commented Oct 25, 2018

Ah, that's unfortunate.. Thanks for checking!

@tsloughter
Copy link
Member

@manuel-rubio any chance you could compare what cowboy sends to what elli sends so we might be able to fix this?

@manuel-rubio
Copy link
Author

manuel-rubio commented Apr 10, 2019

@tsloughter the compression using WebSockets is a bit weird, I have no experience with that sorry, I was only tracing (6 months ago) how worked this. Sorry, I have no idea at this moment what I was trying to do :-D ... but if I have some spare time in the following weeks I promise I'll try to provide more information about it.

@tsloughter
Copy link
Member

Thanks, no worries if not, I just figured I'd ask before seeing if I can reproduce when I get some time.

@starbelly
Copy link

starbelly commented Sep 4, 2019

I have run into this issue with recent ventures with elli_websocket. The problem appears to be around

handle_event(Req, Handler, websocket_open,
.

The client(s) I've tested with do not use the extension and elli_websocket does not return those headers. It seems to be that before the handshake is over, you can write to the socket, resulting in this error under certain circumstances. Namely, if a client is trying to connect while things are still "warming up" is the only way I can make this happen. Specifically, if I fire up an app and concurrently make a WS connection it happens. If I start the app, wait a few seconds and then try to connect, never fails.

It should be noted that the message that is trying to be pushed to the client happens because the client is subscribed to a topic in init or websocket_open event. I think a good solution would be to add something to handler state so that the handler can check if the socket is ready. Either that or queue messages in elli_ws_protocol until things are "ready".

@yurrriq
Copy link
Member

yurrriq commented Sep 5, 2019

Thanks for the feedback. We'd happily review a PR of either (or both) approach(es), if anyone has the time and interest. At first glance, I think I'd lean toward augmenting the handler state, but queuing could be good too, and wouldn't (necessarily) break the current API, in case that's a goal.

@starbelly
Copy link

Yes, I can try to put a PR together. I would be more inclined to augment state as it ultimately should not be a responsibility of elli_websocket IMO, but it may look a bit different once I look into the code a bit.

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

No branches or pull requests

4 participants