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

login error but no logging #5190

Closed
AcrazyDog opened this issue Mar 25, 2021 · 9 comments · Fixed by #5456
Closed

login error but no logging #5190

AcrazyDog opened this issue Mar 25, 2021 · 9 comments · Fixed by #5456
Labels
area/Client Related to Nacos Client SDK contribution welcome kind/user experience not necessarily an error but can be improved for user experience
Milestone

Comments

@AcrazyDog
Copy link

class in com.alibaba.nacos.client.security.SecurityProxy :

    public boolean login(List<String> servers) {

        try {
            if ((System.currentTimeMillis() - lastRefreshTime) < TimeUnit.SECONDS.toMillis(tokenTtl - tokenRefreshWindow)) {
                return true;
            }

            for (String server : servers) {
                if (login(server)) {
                    lastRefreshTime = System.currentTimeMillis();
                    return true;
                }
            }
        } catch (Throwable ignore) {
        }

        return false;
    }

when login error ,catch exception but no logging,user can't identify where is wrong quickly .

@KomachiSion KomachiSion added kind/user experience not necessarily an error but can be improved for user experience area/Client Related to Nacos Client SDK contribution welcome labels Mar 26, 2021
@KomachiSion KomachiSion modified the milestones: 1.4.2, 2.0.1 Mar 26, 2021
@ShawnSiao
Copy link
Contributor

@i will solve it@

@haoyann
Copy link
Collaborator

haoyann commented Apr 12, 2021

login methed catch Exception, if it will throw Throwable, I think replace Exception here a with Throwable better.

@KomachiSion
Copy link
Collaborator

Welcome to enhance it like #5235 without code style problem.

@brotherlu-xcq
Copy link
Collaborator

I will try to solve it.

@brotherlu-xcq
Copy link
Collaborator

I reviewed the login code, I think we catch the Throwable is unnecessary. the main logic in login


method has catch the Exception and return false. If an error was maked, I think the follow codes after the login method shouldn't be ran. What's your opinion? @KomachiSion @haoyann

@KomachiSion
Copy link
Collaborator

KomachiSion commented Apr 22, 2021

If we remove this catch, whether will stop the schedule of scheduleWithFixedDelay?

If the async thread or schedule will be stopped. we must catch it. even we do not care it.

@brotherlu-xcq
Copy link
Collaborator

brotherlu-xcq commented Apr 22, 2021

Yes, if there is an Exception. I think we should catch it, and keep the schedule task can re-run it. but if an Error occured, I am not sure need we stop it and make user to know the Error? usually, An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch. If we need catch the Error as well, I will change my codes to re-catch the throwable and print the error detail.

@KomachiSion
Copy link
Collaborator

There must be retry for login, I think. It is a protection for client.

@brotherlu-xcq
Copy link
Collaborator

OK, thank you. I will change this later.

KomachiSion pushed a commit that referenced this issue Apr 23, 2021
* remove the unnecessary catch in the login method

* catch the throwable error when login and print the message
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/Client Related to Nacos Client SDK contribution welcome kind/user experience not necessarily an error but can be improved for user experience
Projects
None yet
5 participants