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

Disable read timeout for Pub/Sub connections #297

Closed
wants to merge 2 commits into from

Conversation

stevenh
Copy link
Collaborator

@stevenh stevenh commented Nov 22, 2017

Due to the nature Pub/Sub connections they highly likely to block on reads for long periods hence it doesn't make sense to apply any read timeout configured on the pool to Pub/Sub connections.

Due to the nature Pub/Sub connections they highly likely to block on reads for long periods hence it doesn't make sense to apply any read timeout configured on the pool to Pub/Sub connections.
@stevenh
Copy link
Collaborator Author

stevenh commented Nov 22, 2017

For reference the alternative to this would be to test errors return by PubSubConn.Receive() in all consumers to check for net.Error.Timeout() == true and ignore them, which while possible isn't very idiomatic.

Implemented ReceiveNoReadTimeout required by Conn since the change to disable read timeout on Pub/Sub connections.
@garyburd
Copy link
Member

I am rejecting this PR because this is a breaking change. Also, ReadWithoutTimeout is not useful in a robust application.

PING with a duration shorter than the read timeout.

@garyburd garyburd closed this Nov 22, 2017
@stevenh
Copy link
Collaborator Author

stevenh commented Nov 22, 2017

I suspected this may be the case, something else to try an solve with the high level API

We'll keep this internally so we don't have to update all consumers until the new Pub/Sub API is available.

@garyburd
Copy link
Member

See #298

@gomodule gomodule locked and limited conversation to collaborators Aug 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants