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

PUBLISH packet should NOT update the outgoing queue #30

Conversation

gnought
Copy link
Collaborator

@gnought gnought commented Jul 15, 2019

Broker uses brokerCounter to track how many packets it emits, as messageId will be rotated after it reaches max 65535. If there are two PUBLISH packets which brokerCounter is different but same messageId, the second PUBLISH will update the first slot in the queue, as outgoingUpdate is using for loop starting at index 0.

We use outgoingEnqueue or outgoingEnqueueCombi to enqueue PUBLISH packets, and and use outgoingUpdate to update those PUBLSIH items to next state (say PUBREL), so outgoing array should not contain PUBLISH

Broker uses `brokerCounter` to track how many packets it emits, as `messageId` will be rotated after it reaches max 65535. If there are two PUBLISH packets which brokerCounter is different but same messageId, the second PUBLISH will update the first slot in the queue, as `outgoingUpdate` is using `for` loop starting at index 0. This will fix the issue. We use `outgoingEnqueue` or `outgoingEnqueueCombi` to enqueue PUBLISH packets, and and use `outgoingUpdate` to update those PUBLSIH items to next state (say PUBREL), so `outgoing` array should not contain PUBLISH, and that's why this issue could be fixed by a check `packet.cmd !== 'publish'`
Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this going to cause the other persistences to fail?

abstract.js Outdated Show resolved Hide resolved
@gnought
Copy link
Collaborator Author

gnought commented Jul 15, 2019

What other persistences do you want to check?

@gnought
Copy link
Collaborator Author

gnought commented Jul 15, 2019

aedes-persistence-mongodb https://github.com/mcollina/aedes-persistence-mongodb
inherits from aedes-cached-persistence

aedes-persistence-redis https://github.com/mcollina/aedes-persistence-redis
inherits from aedes-cached-persistence

aedes-cached-persistence https://github.com/mcollina/aedes-cached-persistence
does not depend on aedes-persistence

They all treat aedes-persistence functions as interfaces, and implemented by themselves. Quite confident it won't affect those.

@mcollina
Copy link
Collaborator

what's the difference between this and #31?

@gnought
Copy link
Collaborator Author

gnought commented Jul 15, 2019

#31 is a superset of #30

I just take a snapshot of my develop branch for a review.
If #30 is not good, #31 is required to change also. :)

@gnought
Copy link
Collaborator Author

gnought commented Jul 15, 2019

Just wanna submit a small change for ease to have a review. :)

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update aedes-packet as well? I'm going to bump a major.

.travis.yml Show resolved Hide resolved
@mcollina
Copy link
Collaborator

Can we close this and I review #31 instead?

@gnought
Copy link
Collaborator Author

gnought commented Jul 16, 2019

sure.

@gnought gnought closed this Jul 16, 2019
@gnought gnought deleted the hotfix/no_update_queue_same_message_id_in_publish branch July 17, 2019 19:08
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.

2 participants