-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add timeout to RMQ #87
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing the reason why and this may break things
b17b7aa
to
b433348
Compare
Requests taking too long and never timing out could cause incidents, hence adding a configurable timeout. In case the timeout isn't added, no timeout is enforced.
b433348
to
be7286c
Compare
@indrekj Added the reason, changed the code in a way that should not affect its current usage. I'm having trouble creating tests for it, though. Do you have any suggestions? |
# end | ||
# end | ||
def respond_to(destination, &callback) | ||
def respond_to(destination, options = {}, &callback) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options is not documented
I'm still not convinced that having this kind of high-level timeout is a good idea. See:
Maybe if it's large enough and is triggered on very exceptional cases, it could be ok. cc @sviik |
Hm... If we would go with a timeout then that has to end up as an error and also in the sentry (and opsgenie). The current "logger.warn" will get ignored and if we get into a corrupted state then that would be really bad. We probably would need to make sure the error reaches the actual application and then the application can send it to Sentry. I also looked into Rack. We don't have a timeout there either. Rack has "Rack::Timeout" module, but it also warns about the corrupt state. One solution to avoid that is to restart the whole process on timeout. I'm leaning toward not adding the timeout at all. Or if you do, then let's start on the application level, instead of adding it to the library immediately. And we probably should restart the process. |
Requests taking too long and never timing out could cause incidents,
hence adding a configurable timeout. In case the timeout isn't added,
no timeout is enforced.