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: error when unconfirmed message not found #166

Closed

Conversation

luddd3
Copy link
Collaborator

@luddd3 luddd3 commented Aug 23, 2021

Fixes #152

An error is thrown when a message isn't found in _unconfirmedMessages. I don't have a working example of when it might happen, but when I tested around I could reproduce it with reconnects. This is in line with what michaelfitzhavey's team mates found.

Even if we are unsure how it happens, I think it is fair to say that it is better if it doesn't throw an error, since the error states that the message that we want to remove already is removed =)

@luddd3 luddd3 changed the title fix: error when unconfirmed message not found #152 fix: error when unconfirmed message not found Aug 24, 2021
@jwalton
Copy link
Owner

jwalton commented Aug 24, 2021

My concern here is, if the message isn't in _unconfirmedMessages, where is it? Is it maybe in _messages, queued to be re-sent? I'm wondering if this is some corner case where we are requeing the messages because the connection dropped, but then some of the messages actually got sent and we got the confirm "late". If this is the case, we'd end up re-sending this message, even though it was already sent. Maybe we want to search for the message in _messages if it's not in _unconfirmedMessages? And error if it's in neither?

There's some underlying bug here, and I don't like it. :P

@luddd3
Copy link
Collaborator Author

luddd3 commented Aug 25, 2021

I think you might be right. I'm closing this pull request either way since you have rewritten it in TypeScript now. I will create and open another pull request if I find something more tangible.

@luddd3 luddd3 closed this Aug 25, 2021
@luddd3 luddd3 deleted the fix/unconfirmed-message-not-found branch August 27, 2021 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removing unconfirmed message always results in an error
2 participants