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

Fix: Shutdown hub when transport is gone #25

Merged
merged 9 commits into from
Nov 6, 2023
Merged

Conversation

bpolaszek
Copy link
Owner

@bpolaszek bpolaszek commented Jul 25, 2023

Fix #24 - Hub is not aware of redis connection losses

  • Introduces a regular ping to Redis connection
  • Whenever a ping fails, kill the hub

Unfortunately I was not able to set up an automatic reconnection.
Updates can be prevented from being published by returning a 503 error, but for subscriptions, I could not manage to :

  • Stop the SSE
  • Make it aware of a new redis connection

Killing the hub is the best I can do so far - a supervisor process (systemd, supervisor etc) should handle its respawn.

@bpolaszek bpolaszek added the bug Something isn't working label Jul 25, 2023
@codecov
Copy link

codecov bot commented Jul 25, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6fc70c7) 100.00% compared to head (512df63) 100.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##                main       #25   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       138       141    +3     
===========================================
  Files             25        25           
  Lines            396       409   +13     
===========================================
+ Hits             396       409   +13     
Files Coverage Δ
src/Hub/Controller/PublishController.php 100.00% <100.00%> (ø)
src/Hub/Hub.php 100.00% <100.00%> (ø)
src/Hub/Transport/Redis/RedisTransport.php 100.00% <100.00%> (ø)
src/Hub/Transport/Redis/RedisTransportFactory.php 100.00% <100.00%> (ø)
src/functions.php 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bpolaszek bpolaszek marked this pull request as ready for review July 26, 2023 09:02
@bpolaszek bpolaszek mentioned this pull request Jul 26, 2023
@bpolaszek
Copy link
Owner Author

I just added a commit - can you please try again with this fix?
I assume it's already done but don't forget to configure pingInterval in your DSN for this to work.

@misaert
Copy link
Collaborator

misaert commented Nov 2, 2023

I think I don't need to configure pingInterval because it is defined to 2 seconds by default, right?

The new commit seems to fix the issue 👍

But I think the changes on src/Hub/Controller/PublishController.php is unnecessary. WDYT?

@bpolaszek
Copy link
Owner Author

I think I don't need to configure pingInterval because it is defined to 2 seconds by default, right?

The new commit seems to fix the issue 👍

But I think the changes on src/Hub/Controller/PublishController.php is unnecessary. WDYT?

Good news! 🎉
IMO throwing an exception when publishing fails adds an extra layer of reliability as it does not require to wait for the ping to fail (if configured) to throw an error. Besides, this ensures that the hub does not respond with a 201 status when publishing actually failed. WDYT?

@bpolaszek bpolaszek merged commit a868e3b into main Nov 6, 2023
9 checks passed
@bpolaszek bpolaszek deleted the fix/gone-transport branch November 6, 2023 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RedisTransport on close
2 participants