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

There might be non-PUBLISH packets in the send buffer #19

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

smurfix
Copy link
Contributor

@smurfix smurfix commented Nov 8, 2024

Specifically I saw this problem with a Subscribe packet, due to #17, but nothing prevents this case from happening for other reasons.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 11742929093

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 82.816%

Totals Coverage Status
Change from base Build 11742579634: 0.0%
Covered Lines: 6699
Relevant Lines: 8089

💛 - Coveralls

@agronholm
Copy link
Owner

I'm not sure that any non-publish packets should be resent. I need to understand why there are such packets in the buffer (I thought there wouldn't be, hence the assert).

@smurfix
Copy link
Contributor Author

smurfix commented Nov 9, 2024

Consider what happens when the client tries to (un)subscribe to some topic, but the server dies and disconnects before it can send the reply.

@agronholm
Copy link
Owner

Consider what happens when the client tries to (un)subscribe to some topic, but the server dies and disconnects before it can send the reply.

The client will then reset the client state machine, which either leaves the outbound buffer empty (with clean session enabled, or only the publish packets in it otherwise). I see, however, that self._pending_packets is not cleared in reset() which could lead to a problem.

@agronholm
Copy link
Owner

Thus my thinking right now is that my original code in _handle_packet() was correct, but the code in the reset() method is not.

@agronholm
Copy link
Owner

At any rate, this needs a new test.

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.

3 participants