Reduction of escalus crashes when things go wrong & Timeout parameter accepted #43
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Catch all errors on connection steps * Exit nicely if transport:init() fails * On parse error send an error message to owner rather than crash * BOSH Don't crash on timeout reply to request
philwitty
force-pushed
the
more_xmpp_errors
branch
from
3ad2630
to
20cd2a7
Compare
|
Could you create an empty PR for MongooseIM and ejabberd_tests using your escalus branch? |
erszcz
reviewed
Jan 4, 2018
| @@ -240,27 +241,33 @@ init([Args, Owner]) -> | |||
| Port = proplists:get_value(port, Args, 5280), | |||
| Path = proplists:get_value(path, Args, <<"/http-bind">>), | |||
| Wait = proplists:get_value(bosh_wait, Args, ?DEFAULT_WAIT), | |||
| Timeout = proplists:get_value(timeout, Args, infinity), | |||
There was a problem hiding this comment.
infinity is never a good timeout value. We should approximate infinity with a long, but still finite, number here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See commit messages for changes
Justification:
As soon as target systems started timing out, or didn't exist we were generating error logs with hundreds of thousands of entries due to having a few thousand concurrent escalus connections running. We instead wanted to record these issues using the onX functions passed as parameters. Have tried to change so all failures in setting up the connection are caught under the connection step and returned with the connection step failing. Inside the clients themselves tried to minimize the chance of mostly badmatches happening, reporting what we can back to the owner. Will freely admit some of the decisions are a bit hacky and not very nice but open to ideas on a better way to do this. We just needed to avoid crashes as much as possible in normal operation (which involves things not going to plan!).
Timeout is self explanatory and should have 0 functional changes if ignored.