ActionCable: allow to pass websocket.close args by jasl · Pull Request #51043 · rails/rails

jasl · 2024-02-10T18:19:37Z

This PR allows you to set code and reason when closing a WebSocket connection.
It's very useful when hacking ActionCable.

In addition, I small refactor the file to avoid potential nil error, I can remove this change.

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

palkan · 2024-02-13T22:54:20Z

actioncable/lib/action_cable/connection/web_socket.rb


-      def close
-        websocket.close
+      def close(code = nil, reason = nil)


This is an internal method, we do not expose it anywhere.

Thus, I'd suggest to hack around Connection#close in your codebase instead.

I think this is OK because:

According to the implementation, the @websocket can only be ClientSocket

This class is a wrapper, and my patch is just adding the missing args

Could you give me some hints on how to hack Connection#close to customize the code and reason?

With this change, you still need to access internals, e.g., self.websocket.close(code, reason), but you can already do this via self.websocket.websocket.close(code, reason).

Could you give me some hints on how to hack Connection#close to customize the code and reason?

Smth like:

def close(ws_code: nil, ws_reason: nil, **) return websocket.close(ws_code, ws_reason) if ws_code super(**) end # and then connection.close(ws_code: 1001, ws_reason: "whatever")

I don't mean to change the Connection#close interface.

I can override the Connection#close method with my change.

class MyConnection < ActionCable::Connection::Base def close code = 1001 # Determine which code should return websocket.close(code) end end

In your code

def close(ws_code: nil, ws_reason: nil, **) return websocket.close(ws_code, ws_reason) if ws_code super(**) end

My patch is actually made this line websocket.close(ws_code, ws_reason) work.

The real websocket object allows passing code and reason on close,
it is wrapped by the ActionCable::Connection::WebSocket (the websocket instance variable in the Connection class), which doesn't expose the ability, I'm adding it.

rafaelfranca · 2024-02-15T20:12:38Z

This class isn't public API of the framework so it doesn't make much sense to add options so you can use in your application or library. This class should not be used in application or library because it isn't public.

Now, if this class should be private is another discussion. I'm not so sure. If we expect people to be interacting with that object it should not be. What reasons someone have to interact with this object?

jasl · 2024-02-15T23:34:23Z

This class isn't public API of the framework so it doesn't make much sense to add options so you can use in your application or library. This class should not be used in application or library because it isn't public.

Now, if this class should be private is another discussion. I'm not so sure. If we expect people to be interacting with that object it should not be. What reasons someone have to interact with this object?

Returns a code, and a reason is WebSocket standard Spec MDN

In my case, I'm extending ActionCable to add auto disconnect behavior if the client is idle long enough. The server side can give a clear reason to distinguish would be great.

ActionCable now always returns code 1000 and no reason, which does not fully take advantage of WebSocket specification, and I think it is safe to expose the standard defined ability to the application level, not to block advanced (or abnormal) usage, even though it's a private API.

palkan · 2024-02-16T00:52:54Z

The server side can give a clear reason to distinguish would be great.

Then you should rely on Action Cable protocol here, i.e., send {"type":"disconnect","reason":"idle_timeout"} before closing.

Using Action Cable implies following its conventions; if you want to support non-Action Cable-complient clients, you should use a custom connection class and implement your logic there.

if this class should be private is another discussion

And this discussion happens in #50979 🙂 (tl;dr it should not; we should avoid making Connection/Channel classes dependent on the underlying transport mechanism)

jasl · 2024-02-16T01:04:52Z

if you want to support non-Action Cable-complient clients, you should use a custom connection class and implement your logic there.

If I implement my own Connection, I need to access the websocket object to send custom messages, so it should not be private, and exposing code and reason seems reasonable, because it is standard.

In your PR, I replied I'm working on a PoC that implements the NoStr protocol based on the ActionCable Connection class.
https://github.com/jasl/dephy-relay/blob/main/app/channels/nostr_cable/connection.rb

palkan · 2024-02-16T01:38:54Z

exposing code and reason seems reasonable, because it is standard

For WebSockets, yes (and not widely used — that's why build custom protocols on top of them). I'd prefer to avoid coupling high-level Action Cable concepts with transport implementations. (But I'm not the one making decisions here; at least, yet, right, @rafaelfranca 😁?).

Anyway, the proposed change would allow to avoid the following hacking (kinda summary for those who might join the discussion and me thinking out load):

websocket.send(:websocket).close(code, reason)

And instead have:

websocket.close(code, reason)

The websocket object is currently available within a Connection class, it's websocket.websocket that is private. Make the latter one public definitely doesn't make any sense, it's wrapped for a reason (though I have no idea which one).

Since we name the wrapper object a "websocket", it sounds reasonable to support the code and reason arguments. Then, the only I would suggest is to change the proposed implementation to the following:

def close(...)
  websocket.close(...)
end

rafaelfranca · 2024-02-16T21:18:43Z

For WebSockets, yes (and not widely used — that's why build custom protocols on top of them). I'd prefer to avoid coupling high-level Action Cable concepts with transport implementations. (But I'm not the one making decisions here; at least, yet, right, @rafaelfranca 😁?).

If decision are well based like this one, I'm glad that you are trying to make it.

It seems we should expose that websocket object. I agree we should be using the (...) operator as well, so can you change that?

…ection

jasl · 2024-02-16T21:39:41Z

For WebSockets, yes (and not widely used — that's why build custom protocols on top of them). I'd prefer to avoid coupling high-level Action Cable concepts with transport implementations. (But I'm not the one making decisions here; at least, yet, right, @rafaelfranca 😁?).

If decision are well based like this one, I'm glad that you are trying to make it.

It seems we should expose that websocket object. I agree we should be using the (...) operator as well, so can you change that?

Changed, and I also refactored transmit(data) with the same style. If you don't like it, I can revert

palkan

Thanks @jasl!

LGTM 👍

rails-bot bot added the actioncable label Feb 10, 2024

jasl changed the title ~~Ac expose close args~~ ActionCable: allow to pass websocket.close args Feb 10, 2024

palkan reviewed Feb 13, 2024

View reviewed changes

jasl added 3 commits February 17, 2024 05:28

ActionCable: Allow pass code and reason when closing a WebSocket conn…

0df0947

…ection

ActionCable: avoid potential nil error

08cb349

ActionCable: Use ... to passthrough args

d5786b2

jasl force-pushed the ac-expose-close-args branch from 08ef21e to d5786b2 Compare February 16, 2024 21:28

palkan reviewed Feb 16, 2024

View reviewed changes

rafaelfranca merged commit 44db6bb into rails:main Feb 16, 2024

jasl deleted the ac-expose-close-args branch February 16, 2024 22:23

palkan mentioned this pull request Apr 16, 2024

Action Cable server adapterization #50979

Open

5 tasks

Conversation

jasl commented Feb 10, 2024

Checklist

Uh oh!

palkan Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

jasl Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

palkan Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

jasl Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

jasl Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

rafaelfranca commented Feb 15, 2024

Uh oh!

jasl commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

palkan commented Feb 16, 2024

Uh oh!

jasl commented Feb 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

palkan commented Feb 16, 2024

Uh oh!

rafaelfranca commented Feb 16, 2024

Uh oh!

jasl commented Feb 16, 2024

Uh oh!

palkan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jasl commented Feb 15, 2024 •

edited

Loading

jasl commented Feb 16, 2024 •

edited

Loading