Skip to content

Refactor disconnects – avoid JSON in reason #221

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
Merged
merged 3 commits into from
Jan 7, 2022
Merged

Refactor disconnects – avoid JSON in reason #221

merged 3 commits into from
Jan 7, 2022

Conversation

@FZambia
Copy link
Member

@FZambia FZambia commented Jan 4, 2022 •
edited
Loading

Related #149

New Disconnect behaviour described in detail in code comments. Copying from there:

Disconnect allows configuring how client will be disconnected from a server.
A server can provide a Disconnect.Code and Disconnect.Reason to a client. Clients
can execute some custom logic based on a certain Disconnect.Code. Code is also
used for metric collection. Disconnect.Reason is optional and exists mostly for
human-readable description of returned code – i.e. for logging, debugging etc.

The important note is that Disconnect.Reason must be less than 127 bytes
due to WebSocket protocol limitations.

Codes have some rules which should be followed by a client connector implementation.
These rules described below.

Codes in range 0-2999 should not be used by a Centrifuge library user. Those are
reserved for the client-side and transport specific needs. Codes in range >=5000
should not be used also. Those are reserved by Centrifuge.

Client should reconnect upon receiving code in range 3000-3499, 4000-4499, >=5000.
For codes <3000 reconnect behavior can be adjusted for specific transport.

Codes in range 3500-3999 and 4500-4999 are application terminal codes, no automatic
reconnect should be made by a client implementation.

Library users supposed to use codes in range 4000-4999 for creating custom
disconnects.

Some disconnect codes changed here to match new semantics, but since we never exposed codes to client's DisconnectContext it seems pretty safe. Reconnect field removed – but we still support building old-fashioned Reason for ProtocolVersion1.

Clients will need to expose code only for ProtocolVersion2.

For Centrifugo this will be a breaking change when custom Disconnects are used – but this should be possible to adopt fully on the backend side.

Eventually Disconnect.CloseText(ProtocolVersion) should be removed.

@FZambia FZambia changed the base branch from master to new_protocol January 4, 2022 23:13
@codecov
Copy link

codecov bot commented Jan 4, 2022 •
edited
Loading

Codecov Report

Merging #221 (6a73176) into new_protocol (a3f8c72) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 6a73176 differs from pull request most recent head 44aa465. Consider uploading reports for the commit 44aa465 to get more accurate results
Impacted file tree graph

@@               Coverage Diff                @@
##           new_protocol     #221      +/-   ##
================================================
- Coverage         84.44%   84.41%   -0.03%     
================================================
  Files                30       30              
  Lines              5315     5325      +10     
================================================
+ Hits               4488     4495       +7     
- Misses              538      541       +3     
  Partials            289      289
Impacted Files Coverage Δ
client.go 84.49% <100.00%> (+0.18%) ⬆️
disconnect.go 100.00% <100.00%> (ø)
handler_sockjs.go 74.48% <100.00%> (ø)
handler_websocket.go 79.48% <100.00%> (ø)
internal/dissolve/queue.go 94.82% <0.00%> (-5.18%) ⬇️
broker_redis.go 72.93% <0.00%> (-0.50%) ⬇️
hub.go 85.17% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3f8c72...44aa465. Read the comment docs.

@FZambia FZambia changed the title Disconnects for protocol v2 Refactor disconnects – avoid JSON in reason Jan 5, 2022
@FZambia FZambia merged commit eb20161 into new_protocol Jan 7, 2022
@FZambia FZambia deleted the disconnects_for_protocol_v2 branch January 7, 2022 06:24
@FZambia FZambia mentioned this pull request Jan 7, 2022
Merged
2 tasks
This was referenced Jan 15, 2022
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@FZambia