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

producer: handle case of no transactions in popTransaction() #346

Merged
merged 1 commit into from
Aug 1, 2022

Conversation

ploxiln
Copy link
Member

@ploxiln ploxiln commented Jun 5, 2022

This corner case generally should not happen, but a nsqd bug or
strange network condition could possibly send erroneous bytes that
are interpreted as a response. The Producer should be robust,
and not panic the whole process.

fixes #345 (hopefully)

@mreiferson
Copy link
Member

yea, it makes sense to be defensive here, but let's also log the data so if/when we catch this in the wild we better understand what's happening?

@mreiferson mreiferson added the bug label Jun 11, 2022
@yiippee
Copy link

yiippee commented Jul 8, 2022

@ploxiln You said the producer is "thread-safe", but how it is guaranteed to get responses in the order which commands were sent? The nsqd guarantees it? Thanks.

func (w *Producer) popTransaction(frameType int32, data []byte) {
	t := w.transactions[0]  // How can you be sure it's the first one element.  
	w.transactions = w.transactions[1:]
	if frameType == FrameTypeError {
		t.Error = ErrProtocol{string(data)}
	}
	t.finish()
}

@ploxiln
Copy link
Member Author

ploxiln commented Jul 8, 2022

how it is guaranteed to get responses in the order which commands were sent? The nsqd guarantees it?

Yes, in this way it's similar to http request "pipelining"

@mreiferson
Copy link
Member

@ploxiln can you update this as per #346 (comment)

@ploxiln
Copy link
Member Author

ploxiln commented Jul 31, 2022

OK - how does that look? (first 32 bytes in hex)

This corner case generally should not happen, but a nsqd bug or
strange network condition could possibly send erroneous bytes that
are interpreted as a response. The Producer should be robust,
and not panic the whole process.
@ploxiln ploxiln merged commit 0e8d7a7 into nsqio:master Aug 1, 2022
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 this pull request may close these issues.

Panic in producer's router
3 participants