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

remove logic that stops gw loading until emergency mode is triggered or rpc conn is successful #6890

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sredxny
Copy link
Contributor

@sredxny sredxny commented Feb 19, 2025

User description

TT-14089
Summary Gateway: Cold start does not initialize gateway after restart
Type Bug Bug
Status In Test
Points N/A
Labels -

Description

Related Issue

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

  • I ensured that the documentation is up to date
  • I explained why this PR updates go.mod in detail with reasoning why it's required
  • I would like a code coverage CI quality gate exception and have explained why

PR Type

Bug fix


Description

  • Removed logic that delays gateway loading until emergency mode or RPC connection.

  • Simplified the Connect function by removing waitForClientReadiness.

  • Ensured gateway initialization proceeds without waiting for RPC readiness.


Changes walkthrough 📝

Relevant files
Bug fix
rpc_client.go
Removed client readiness wait logic in RPC connection       

rpc/rpc_client.go

  • Removed the waitForClientReadiness function entirely.
  • Updated Connect function to no longer wait for client readiness.
  • Simplified the flow for handling login and registration.
  • +1/-19   

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @buger
    Copy link
    Member

    buger commented Feb 19, 2025

    Knock Knock! 🔍

    Just thought I'd let you know that your PR title and story title look quite different. PR titles that closely resemble the story title make it easier for reviewers to understand the context of the PR.

    An easy-to-understand PR title a day makes the reviewer review away! 😛⚡️
    Story Title Gateway: Cold start does not initialize gateway after restart
    PR Title remove logic that stops gw loading until emergency mode is triggered or rpc conn is successful

    Check out this guide to learn more about PR best-practices.

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Issue

    The removal of waitForClientReadiness logic may lead to unexpected behavior if the client is not ready before handleLogin is called. Ensure that the new flow handles client readiness appropriately.

    if funcClientSingleton == nil {
    	funcClientSingleton = dispatcher.NewFuncClient(clientSingleton)
    }
    
    handleLogin()
    Code Simplification

    The waitForClientReadiness function has been removed, but its functionality might still be useful for ensuring readiness. Consider if a simplified version of this logic is necessary.

    		go checkDisconnect()
    	}
    
    	return true
    }
    
    func handleLogin() {
    	if UseSyncLoginRPC == true {
    		Login()
    		return

    Copy link
    Contributor

    API Changes

    no api changes detected

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Prevent race conditions in client readiness

    Ensure that removing the waitForClientReadiness logic does not introduce race
    conditions or unhandled states where the client is not ready before handleLogin is
    called.

    rpc/rpc_client.go [307]

    -handleLogin()
    +if clientSingleton.IsReady() {
    +    handleLogin()
    +} else {
    +    log.Error("Client not ready before login")
    +}
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion addresses a potential race condition introduced by the removal of waitForClientReadiness. By checking if the client is ready before calling handleLogin, it ensures that the client is in a valid state, preventing possible errors or unhandled states. This is a significant improvement in terms of robustness and correctness.

    Medium

    Copy link

    Quality Gate Failed Quality Gate failed

    Failed conditions
    11 Security Hotspots
    E Security Rating on New Code (required ≥ A)

    See analysis details on SonarQube Cloud

    Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants