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(model-server-lib): increase websocket timeouts #208

Closed
wants to merge 1 commit into from

Conversation

languitar
Copy link
Contributor

The existing timeouts sometimes caused disconnects when requesting larger amounts of data despite everything being functional.

The existing timeouts sometimes caused disconnects when requesting
larger amounts of data despite everything being functional.
@languitar languitar requested a review from a team as a code owner August 17, 2023 09:08
@odzhychko
Copy link
Contributor

odzhychko commented Aug 17, 2023

Should we make the timeout configurable?
Ktor provides mechanisms for it https://ktor.io/docs/configuration-file.html
I used them for example here: workspace-manager/src/main/resources/application.conf

@languitar
Copy link
Contributor Author

Configuration would be great. However, whether the configuration file of ktor is read or not depends on whether engineMain is used or not. Moreover, we would then also need a mechanism to configure a custom application configuration in the light-model-server instance running inside MPS.

From what I have seen, most libraries seem to default to 20 seconds for pings, but they usually assume simple tasks where communicating parties are not frequently blocked by cpu-intensive long-running tasks. Therefore, maybe this is still also a sensible default.

@odzhychko
Copy link
Contributor

I see, we use embeddedServer. So the configuration is done in code.

We could add the timeout as field to LightModelServer and LightModelServerBuilder (like port).
The field LightModelServerBuilder could have a default value.
This would make LightModelServer configurable, if it is used outside our MPS plugin.

Of course in the MPS Plugin, where LightModelServer is build. we would still hard code the timeout or decide to use the default.

@languitar
Copy link
Contributor Author

Inside MPS, this would either requiring to go through the MPS config mechanism or to just read some environment variable.

In any case, maybe giving a bit more headroom here by default won't hurt?

@odzhychko
Copy link
Contributor

I understood, that a configuration in the MPS plugin would be much more to implement and currently not needed.
I was just suggesting this main...feature/lmx-timeouts-suggestion to pull out constants of LightModelServer.

@languitar
Copy link
Contributor Author

I understood, that a configuration in the MPS plugin would be much more to implement and currently not needed. I was just suggesting this main...feature/lmx-timeouts-suggestion to pull out constants of LightModelServer.

Looks fine. Do you want to open a PR with that change as a replacement for my PR here?

@odzhychko
Copy link
Contributor

Sure. Closing in favor of #211

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.

2 participants