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: Disable the default timeout for requests on the LU build #4295

Merged
merged 13 commits into from
Nov 17, 2020

Conversation

benbrown
Copy link
Contributor

Description

This removes the timeout behavior on the /build endpoint, which can result in a network error when publishing a large language model. By default, Node's HTTP server class is set to timeout after 2 minutes.

https://nodejs.org/dist/latest-v6.x/docs/api/http.html#http_server_settimeout_msecs_callback

Task Item

fixes #4294

…step which can take > 2 minutes for large models
@benbrown benbrown changed the title Fix #4294 - Disable the default timeout for requests on the LU build fix: Disable the default timeout for requests on the LU build Sep 29, 2020
@a-b-r-o-w-n
Copy link
Contributor

Although this addresses #4294, we should attempt to refactor this endpoint to better fit long processes, like polling or web socket.

@benbrown
Copy link
Contributor Author

I agree @a-b-r-o-w-n and have a proposal that I will follow up with ASAP.

In short: we already implement a polling procedure for the publishing. I would like to formalize this so that publishing, provisioning, and building can take advantage of the same subsystem.

@a-b-r-o-w-n
Copy link
Contributor

@benbrown that's good to hear.

@cwhitten
Copy link
Member

Test environment is failing with:

TypeError: req.setTimeout is not a function

  325 | 
  326 |   // Disable Express' built in 2 minute timeout for requests. Otherwise, large models may fail to build.
> 327 |   req.setTimeout(0, () => {
      |       ^
  328 |     throw new Error('LUIS publish process timed out.');
  329 |   });
  330 | 

Maybe we need to shim this api in the test runner?

@a-b-r-o-w-n a-b-r-o-w-n added the 1.3 1.3 Release label Nov 13, 2020
@cwhitten
Copy link
Member

@benbrown i don't know why tests are failing but it seems specific to this change

@benbrown
Copy link
Contributor Author

@cwhitten I think I found it. Just pushed a small change to the test.

@coveralls
Copy link

coveralls commented Nov 17, 2020

Coverage Status

Coverage decreased (-0.0004%) to 54.465% when pulling ec026c7 on benbrown/fix4294 into 771af33 on main.

@cwhitten cwhitten merged commit 9d01a7a into main Nov 17, 2020
@cwhitten cwhitten deleted the benbrown/fix4294 branch November 17, 2020 21:56
EricDahlvang pushed a commit that referenced this pull request Nov 27, 2020
* Fix #4294 - Disable the default timeout for requests on the LU build step which can take > 2 minutes for large models

* update mocked request object to contain settimeout

Co-authored-by: Chris Whitten <christopher.whitten@microsoft.com>
benbrown added a commit to benbrown/BotFramework-Composer that referenced this pull request May 24, 2021
…oft#4295)

* Fix microsoft#4294 - Disable the default timeout for requests on the LU build step which can take > 2 minutes for large models

* update mocked request object to contain settimeout

Co-authored-by: Chris Whitten <christopher.whitten@microsoft.com>
benbrown added a commit that referenced this pull request Jun 11, 2021
* Fix #4294 - Disable the default timeout for requests on the LU build step which can take > 2 minutes for large models

* update mocked request object to contain settimeout

Co-authored-by: Chris Whitten <christopher.whitten@microsoft.com>
lei9444 pushed a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
…oft#4295)

* Fix microsoft#4294 - Disable the default timeout for requests on the LU build step which can take > 2 minutes for large models

* update mocked request object to contain settimeout

Co-authored-by: Chris Whitten <christopher.whitten@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.3 1.3 Release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Network Error" when publishing a big Luis app
4 participants