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 stream becoming readable again #503

Merged
merged 1 commit into from
Nov 16, 2019

Conversation

johanneswuerbach
Copy link
Contributor

I'm not really sure how to test this for real, but under constant publishing I can trigger an occasional Uncaught AssertionError [ERR_ASSERTION]: 0 == 1 https://github.com/squaremo/amqp.node/blob/c28c176dd74565c73957eebf00932a892d2aa840/lib/mux.js#L92

It seems that some kind of stream in this library is emitting readable, during read calls and causing this assertion to trip.

@squaremo
Copy link
Collaborator

I think I understand what the problem is; I need to think more about the solution though

@johanneswuerbach
Copy link
Contributor Author

@squaremo sorry for the ping, but do you have any update? :-)

@johanneswuerbach
Copy link
Contributor Author

@squaremo anything I could do to help moving this forward?

@squaremo
Copy link
Collaborator

squaremo commented Aug 3, 2019

Right I see, the problem is that (as you say) a stream can be added back into newStreams after the initial loop exits, and before the assertion that newStreams is empty. The loop exits because a null chunk has been read (indicating no more chunks); but, 'readable' fires (synchronously, somehow) and the handler for that event appends the stream to newStreams. This breaks the invariant that either all newStreams are exhausted (and removed from that queue), or the downstream is full.

Your fix restores the invariant by putting another loop around the problematic loop, to catch the case where the loop exits but the stream has been added back by the 'readable' event handler.

I think another way to fix it would be to simply remove the special case for when there's a single stream. Since that loop will only exit when !accepting (as before) or the queue is empty, it will have the same effect as the extra loop. It's possible, but harmless, for a stream to be added back to the queue twice.

@johanneswuerbach
Copy link
Contributor Author

Sorry for the late response @squaremo, but now updated the PR with your suggestion and it still works for us 👍

@johanneswuerbach
Copy link
Contributor Author

@squaremo sorry for the ping, but is there anything else required here?

Copy link
Collaborator

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

Thank you for figuring this out @johanneswuerbach! It's nice that it ended up removing code too 🍫

@squaremo squaremo merged commit 90e0dc0 into amqp-node:master Nov 16, 2019
@johanneswuerbach
Copy link
Contributor Author

@squaremo thank you for the support. Could you also release a new version with this fix? That would be wonderful.

@axnsan12
Copy link

@squaremo can you please publish a release with this fix?

@glebsts
Copy link

glebsts commented May 19, 2020

Why it had no changelog update?

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.

4 participants