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

Compile asynchronously #890

Merged

Conversation

isc-bsaviano
Copy link
Contributor

This PR removes the use of the synchronous /action/compile API and instead uses the /work endpoints to compile documents. We've had reports internally of users running into 504 Gateway Timeout errors when compiling a class took more than 60 seconds. While the users could increase the timeout, they could always run up against it again in the future. Using this approach ensures that the extension will not break, regardless of how long compilation on the server may take.

I'm leaving this as a draft until the people who ran into the gateway timeout issue internally verify that this works for them.

daimor
daimor previously approved these changes Mar 2, 2022
@isc-bsaviano isc-bsaviano marked this pull request as ready for review March 2, 2022 20:56
@isc-bsaviano
Copy link
Contributor Author

@daimor @gjsjohnmurray The internal devs verified that this fixed the 504 error. I think you should both try this out before it gets merged though.

@gjsjohnmurray
Copy link
Contributor

Maybe cancellation should add a message to the output channel, in case the partially-compiled classes on the server result in unexpected behaviour.

@gjsjohnmurray
Copy link
Contributor

@isc-bsaviano as mentioned on today's call, you'll look to address my feedback re a cancel message. I don't see this as a showstopper though, so if @daimor is happy with what's already in this PR but you don't get to the cancel message thing imminently I suggest merging as-is so it can get into the upcoming release.

gjsjohnmurray
gjsjohnmurray previously approved these changes Mar 18, 2022
@isc-bsaviano
Copy link
Contributor Author

@gjsjohnmurray I will add a cancellation message, I think that's good to have

@isc-bsaviano
Copy link
Contributor Author

@gjsjohnmurray If the message looks ok to you I will merge this.

Copy link
Contributor

@gjsjohnmurray gjsjohnmurray left a comment

Choose a reason for hiding this comment

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

👍

@isc-bsaviano isc-bsaviano merged commit 923c6c9 into intersystems-community:master Mar 18, 2022
@isc-bsaviano isc-bsaviano deleted the async-compile branch March 18, 2022 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants