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

Issue #762: Remove calls to log.Fatal #763

Merged
merged 3 commits into from
Jan 11, 2025
Merged

Issue #762: Remove calls to log.Fatal #763

merged 3 commits into from
Jan 11, 2025

Conversation

danomagnum
Copy link
Contributor

@danomagnum danomagnum commented Jan 10, 2025

The function that used log.Fatal in config.go isn't exported and is only called by functions that return errors so I added an error return then pass it up.

The first log.Fatal in server.go followed the same approach as #616 to delay the error till the start() is called on the server.

The second log.Fatal in server.go should never be possible, so I changed it to a panic (which should never happen).

Fixes #762

Copy link
Member

@magiconair magiconair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than that LGTM

server/server.go Outdated Show resolved Hide resolved
@magiconair magiconair enabled auto-merge January 11, 2025 17:38
@magiconair
Copy link
Member

LGTM

@magiconair magiconair merged commit 780b10f into main Jan 11, 2025
5 checks passed
@magiconair magiconair deleted the removeFatals branch January 11, 2025 17:42
@danomagnum
Copy link
Contributor Author

danomagnum commented Jan 11, 2025

@magiconair

Did you see what happened with the checks on this? It failed the first time (couldn't open port 4840 in one of the conn_test tests). It does pass on my machine, so I pushed an empty commit and when it ran again, it passed. Some kind of race condition must exist in terms of one test closing the port fast enough before another can open it? Pretty strange.

On the python integration tests I've started getting a different error sometimes too. I'm sure it's something with how my network is setup, but I'm not 100% sure why it doesn't try to go out the gateway anyway.

=== RUN   TestClientTimeoutViaOptions
Endpoints other than open requested but private key and certificate are not set.
Listening on 0.0.0.0:4840
    timeout_test.go:55: 
                Error Trace:    /home/dan/programs/opcua/tests/python/timeout_test.go:55
                                                        /home/dan/programs/opcua/tests/python/timeout_test.go:28
                Error:          got %#v, wanted net.timeoutError
                Test:           TestClientTimeoutViaOptions
                Messages:       connect: network is unreachable
--- FAIL: TestClientTimeoutViaOptions (1.45s)

@magiconair magiconair changed the title Issue #762: Remove log.Fatal's. Issue #762: Remove calls to log.Fatal Jan 11, 2025
@magiconair
Copy link
Member

I've seen this and it happens sporadically but not often enough for me to be concerned. Would be great to understand why though.

Released as v0.6.3

@magiconair
Copy link
Member

@danomagnum I wonder where all the thumbs-up on the issue came from. People from your company? If this was such an issue why didn't anyone raise this sooner.

@danomagnum
Copy link
Contributor Author

@magiconair I don't know. Not sure who any of the people are. I saw the issue about log.fatal come in and it involved the server so I went ahead and fixed it. That issue got a bunch of attention really quickly as well. Maybe all those people are at the same company?

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.

Very dangerous log.Fatal called in several places
2 participants