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] The process was freezing in some cases when HTTP calls exceeds timeout on integrations #11253

Merged
merged 4 commits into from
Jun 26, 2018

Conversation

rodrigok
Copy link
Member

@rodrigok rodrigok commented Jun 25, 2018

Closes #10062

The problem happens when we have a timeout exception triggered by some async method like the HTTP, this async method is turned sync by the usage of Fibers, for some reason that exception, some times we need more than one, stops the oplog tail method that uses Fibers too leading to a stop of reactivity we use to sync the clients.

The solution was start a new Fiber inside the VM to prevent the exceptions to stop the fibers, in my tests this change solved the problem.

@rodrigok rodrigok added this to the 0.66.0 milestone Jun 25, 2018
@rodrigok rodrigok added this to Review/QA in June/2018 via automation Jun 25, 2018
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11253 June 25, 2018 19:18 Inactive
@sampaiodiego
Copy link
Member

previously, if the HTTP call exceeds the vm time out, the integration execution would failure and a message would not be sent.

with this change, the integration runs successfully after the timeout..

is this an intentional change?

@rodrigok
Copy link
Member Author

@sampaiodiego yes, cuz the async time does not count in the script execution time.

@@ -236,9 +240,19 @@ function executeIntegrationRest() {
sandbox.script = script;
sandbox.request = request;

const result = vm.runInNewContext('script.process_incoming_request({ request: request })', sandbox, {
const result = Future.fromPromise(vm.runInNewContext(`
new Promise((resolve, reject) => {
Copy link
Member

@sampaiodiego sampaiodiego Jun 26, 2018

Choose a reason for hiding this comment

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

please, add a catch to this Promise, otherwise calling reject will thrown a unhandled promise rejection

edit: I've added 👍

@sampaiodiego sampaiodiego temporarily deployed to rocket-chat-pr-11253 June 26, 2018 18:07 Inactive
sampaiodiego
sampaiodiego previously approved these changes Jun 26, 2018
Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

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

I'm approving this, but since it is changing the behavior of how integration scripts failure, I would use a tag [BREAK] instead of [FIX]

@RocketChat RocketChat deleted a comment Jun 26, 2018
@rodrigok
Copy link
Member Author

LGTM

@sampaiodiego sampaiodiego changed the title [FIX] The process was freezing in some cases where HTTP calls exceeds timeout on integrations [FIX] The process was freezing in some cases when HTTP calls exceeds timeout on integrations Jun 26, 2018
@sampaiodiego sampaiodiego merged commit fdf221f into develop Jun 26, 2018
June/2018 automation moved this from Review/QA to Closed Jun 26, 2018
@sampaiodiego sampaiodiego deleted the freeze branch June 26, 2018 20:20
@rodrigok rodrigok mentioned this pull request Jun 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
June/2018
  
Closed
Development

Successfully merging this pull request may close these issues.

Long-running outgoing webhook breaks sending of messages
3 participants