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

Program hangs where token is missing or is invaild without any feedback #176

Open
swalx opened this issue Jul 24, 2024 · 3 comments
Open

Comments

@swalx
Copy link

swalx commented Jul 24, 2024

When starting an http endpoint, the program hangs (seems indefinitely) if the token is invalid or does not exists.

At first I thought simply checking if the environment variable exists and that it has a value would resolve the issue. However, this was not the case.
image

I understand that the Connect func blocks until success but shouldn't it also return an error? In this case, the ngrok service should at least return an authentication error.

// Connect begins a new ngrok [Session] by connecting to the ngrok service,
// retrying transient failures if they occur.
//
// Connect blocks until the session is successfully established or fails with
// an error that will not be retried. Customize session connection behavior
// with [ConnectOption] arguments.
func Connect(ctx context.Context, opts ...ConnectOption) (Session, error) {

Whether or not this is expected behaviour (for it to hang), I want to believe that where an attempt to start an http endpoint fails due to a missing or invalid token. The most appropriate one of the following errors should be returned to the user: ERR_NGROK_105, ERR_NGROK_106 or ERR_NGROK_107. This would give the user enough feedback for them to make any necessary changes.

This can be reproduced by following the instructions exactly as seen in the Quickstart example found in the README with or without the example token.

This library is really awesome and useful. I just decided to test it out and realise this problem. If I am missing something, it would be great if I could be pointed in the right direction. Otherwise, I will continue to familiarise myself with the code base to potentially open a PR with a fix (assuming no one would have already done so).

Similar issue but includes the error: #107

swalx pushed a commit to swalx/ngrok-go that referenced this issue Jul 24, 2024
Missing return for second case caused programs to hang where
authtoken is missing or is invalid without showing any feedback.

Fixes: ngrok#176
@CK-Ward
Copy link
Contributor

CK-Ward commented Sep 5, 2024

Hi @swalx, thank you for bringing this to our attention. Are you still experiencing this issue? We'll have our team take a look.

@jrobsonchase
Copy link
Collaborator

It's not quite that there's no feedback, it's just somewhat hidden. Each time a connection is attempted and fails, or the session is disconnected, the DisconnectHandler is called with the error.

	sess, err := ngrok.Connect(ctx,
		ngrok.WithAuthtokenFromEnv(),
		ngrok.WithDisconnectHandler(func(ctx context.Context, sess ngrok.Session, err error) {
			if err != nil {
				fmt.Println("error: ", err)
			}
		}),
	)

Additionally, ngrok.Connect will give up and return an error if the reconnect loop stops. Currently, this only occurs when the session is closed, which you can also do via the disconnect handler:

	sess, err := ngrok.Connect(ctx,
		ngrok.WithAuthtokenFromEnv(),
		ngrok.WithDisconnectHandler(func(ctx context.Context, sess ngrok.Session, err error) {
			if err != nil {
				sess.Close()
			}
		}),
	)
	if err != nil {
		fmt.Println("ngrok.Connect failed!")
		return err
	}

We could definitely improve on the out-of-the-box experience with a default disconnect handler which makes an attempt to classify errors as retryable or not, and kill the session accordingly.

@swalx
Copy link
Author

swalx commented Sep 6, 2024

@CK-Ward, thank you for your response and for looking into this. My usage have been somewhat limited as of recent. However, I had identified (2928705) what I believed to have been the problem. Which was a missing early return statement. Upon added said statement, the issue was resolved and the expected error messages were being returned.

A PR was opened with the potential fix for review: #177

I wanted to add a reviewer or assign someone to it but none of those options were available and I completely forgot I could just mention it here.

I did an upgrade to v1.10.0 just now in the same quickstart example and the issue seems to still be present.

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

No branches or pull requests

3 participants