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

Router UpdateServer function doesn't work as advertised #784

Open
wainglaister opened this issue May 13, 2024 · 3 comments
Open

Router UpdateServer function doesn't work as advertised #784

wainglaister opened this issue May 13, 2024 · 3 comments
Labels
bug Something isn't working internally-reviewed The issue has been reviewed internally.

Comments

@wainglaister
Copy link

Component(s)

router

Component version

router@0.85.2

wgc version

0.53.2

controlplane version

N/A

router version

0.85.2

What happened?

Description

Core router.go provides an exported UpdateServer function whose description says it creates and starts a new server and replaces the old one. This is inaccurate, the new server is not started.
There is a non-exported function (updateServerAndStart) which uses the exported function and does start the new server.

Steps to Reproduce

(by some means) call err := router.UpdateServer(ctx, newRouterConfig)

Expected Result

Calling the exported function will replace the existing server with a newly configured and started server.

Actual Result

Calling the exported function results in an inoperable system who's server exists but is not listening for connections.

Environment information

Environment

Problem is unrelated to environment
Source is this PR: https://github.com/wundergraph/cosmo/pull/446/files#diff-aa58824b56f499a6a7d39dd7d905130a1aed0683be3aa6826a63dfd5d5047edcL373

Router configuration

No response

Router execution config

No response

Log output

15:59:38 PM WARN Advanced Request Tracing (ART) is enabled in development mode but requires a graph token to work in production. For more information see https://cosmo-docs.wundergraph.com/router/advanced-request-tracing-art {"component": "@wundergraph/router", "service_version": "dev"}
15:59:38 PM INFO Serving GraphQL playground {"component": "@wundergraph/router", "service_version": "dev", "url": "http://0.0.0.0:3002/"}
15:59:38 PM INFO Rate limiting disabled {"component": "@wundergraph/router", "service_version": "dev"}
15:59:38 PM INFO GraphQL endpoint {"component": "@wundergraph/router", "service_version": "dev", "method": "POST", "url": "http://0.0.0.0:3002/graphql"}
15:59:38 PM INFO Gracefully shutting down the router ... {"component": "@wundergraph/router", "service_version": "dev", "config_version": "", "grace_period": "20s"}
15:59:38 PM INFO Server stopped {"component": "@wundergraph/router", "service_version": "dev", "config_version": ""}
15:59:42 PM INFO WebSocket Stats {"component": "@wundergraph/router", "service_version": "dev", "open_connections": 0, "active_subscriptions": 0}
15:59:42 PM INFO Memory usage {"component": "@wundergraph/router", "service_version": "dev", "alloc_mb": "6.5 MB", "total_alloc_mb": "20 MB", "sys_mb": "20 MB", "num_gc": 9}

Additional context

Note in the log that the new server does not start listening.

It seems like the source PR (446) split the functions (UpdateServer) and 'broke' the original exported function (compared to its description). It looks like the new non-exported function (updateServerAndStart) should actually be the one that is exported and has the description.

@wainglaister wainglaister added the bug Something isn't working label May 13, 2024
Copy link

WunderGraph commits fully to Open Source and we want to make sure that we can help you as fast as possible.
The roadmap is driven by our customers and we have to prioritize issues that are important to them.
You can influence the priority by becoming a customer. Please contact us here.

@wainglaister
Copy link
Author

PR: #787

@jensneuse
Copy link
Member

Hey, this is an interesting addition to Cosmo Router. We've also seen the PR. We might want to have some modifications but this needs further review. We will address this in due course.

@jensneuse jensneuse added the internally-reviewed The issue has been reviewed internally. label May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working internally-reviewed The issue has been reviewed internally.
Projects
None yet
Development

No branches or pull requests

2 participants