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

Event for failing reconnection #222

Closed
ssukienn opened this issue Jan 7, 2022 · 2 comments · Fixed by #223
Closed

Event for failing reconnection #222

ssukienn opened this issue Jan 7, 2022 · 2 comments · Fixed by #223
Labels

Comments

@ssukienn
Copy link

ssukienn commented Jan 7, 2022

Hi.

With changes based on this issue: #145 (I actually commented there but without reply) I am unable to follow reconnection count without introducing my own timer over libs retry time (plus there is possibility that if target node is unavailable, reconnect timeout will be underlying socket connection timeout, not time from lib options, what creates it more hard to do proper syncing).

I am not against making distinction between disconnect from actually connected "connection" event (new behavior) and not successful reconnection (now missing) event. But introduced changes removed the latter events from the timeline now and it is actually breaking for users who relied on this behavior.

Now my thinking about this is that if library offers the means to reconnect disrupted connection it should also offer the means to monitor this process (it might have some importance for the consumers). We had "connect" and "disconnect" (covering actual disconnect and failed reconnections). Now we have "connect" and "disconnect" (for actual failure of connecting first time or when we get the "close" from amqp) but we miss something like "reconnection-failure".

Whether there should be seperate event or not (payload changes?) is the matter of the discussion but I think there should be events informing about failed reconnections. It is just easier for the library to provide than expect from the users to mimic or approximate. I am open to create PR in free time if we would have some decision how to proceed.

Wdyt?

jwalton added a commit that referenced this issue Jan 7, 2022
fix #222

BREAKING CHANGE: We will no longer emit a `disconnect` event on an
initial connection failure - instead we now emit `connectFailed` on each
connection failure, and only emit `disconnect` when we transition from
connected to disconnected.
@jwalton
Copy link
Owner

jwalton commented Jan 7, 2022

Sorry - missed your comment on #145. But yes, a separate event for connection attempts sounds reasonable to me. How does this sound?

github-actions bot pushed a commit that referenced this issue Jan 7, 2022
# [4.0.0](v3.9.0...v4.0.0) (2022-01-07)

### Bug Fixes

* Emit `connectFailed` on connection failure. ([0f05987](0f05987)), closes [#222](#222)

### Continuous Integration

* Stop testing on node 10 and 12. ([5da9cb0](5da9cb0))

### BREAKING CHANGES

* No longer running unit tests on node 10 and 12, although this package may continue to work on these.
* We will no longer emit a `disconnect` event on an
initial connection failure - instead we now emit `connectFailed` on each
connection failure, and only emit `disconnect` when we transition from
connected to disconnected.
@jwalton
Copy link
Owner

jwalton commented Jan 7, 2022

🎉 This issue has been resolved in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

2 participants