[BUG] 1.5.1 uses log.Printf

[jech]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

Commit 666c197 is a huge commit with no useful log message, and I keep finding new issues with it.

One of the issues is that it included a bunch of error handling using log.Printf. gorilla/websocket is a generally useful library, and it should not be making any assumptions about my application's logging infrastructure; using log.Printf for logging is a clear violation of this basic principle.

Please revert commit 666c197.

Expected Behavior

A low-lever library should not be doing logging on behalf of the application.

Steps To Reproduce

No response

Anything else?

No response

[AlexVulaj]

Hey @jech thanks for bringing this up. Reverting the entire commit isn't likely to happen as a lot of those changes were to bring our codebase in line with new linters and such. However, could you take a look and see if this PR addresses your issue? #878

[jech]

I understand that it's difficult to rollback the entire commit, but is is practical to submit a PR to undo actual functionality changes

I believe that the proper way to proceed would be to revert said commit, and then resubmit the useful parts with proper commit messages. If that is not done, then somebody will need to fork the package.

[FZambia]

I am also not updating to Gorilla WebSocket v1.5.1 in https://github.com/centrifugal/centrifuge and https://github.com/centrifugal/centrifugo to not introduce unwanted noise to user's logs. Is there a plan to revert the changes made? I agree with above comments and will prefer forking the package than migrating to it in the current state.

[ReToCode]

+1 on this, we have the same concerns in Knative: knative/serving#14597.

[AlexVulaj]

Hey all - one of our maintainers submitted this PR to hopefully undo the logging that was added causing all of the extra noise. Hoping to get that pushed through soon.

[jech]

@AlexVulaj, the problem is that since 666c197 is so large and undocumented, it is impossible to review.

If you want us to trust gorilla/websocket again, you need to revert 666c197, and then resubmit the useful changes in manageable units with proper commit messages. Leaving the commit in and then playing whack-a-mole with the issues it introduced is not going to produce sofftware we can depend on.

[AlexVulaj]

Didn't mean to close this issue - must've been an automated process with the merge of the above PR. I'm leaving this open for discussion until the logging issues are confirmed to be good.

[AlexVulaj]

@jech While I totally understand the desire to revert that commit and move forward, our concern is that we'd lose a number of community contributions that have been made since that change. We're trying our best to fix the problems brought up in this thread without erasing any of those valuable contributions, which can be a difficult process.

I appreciate everyone's patience here as we work through this.

[jech]

@AlexVulaj This is not a mere desire. There is simply no way to review that commit, and hence there is no way to convince ourselves that no erroneous or even malicious code has been snuck into Gorilla websocket.

It is simply not possible for us to trust this branch of Gorilla Websocket as long as this commit is not reverted and the features submitted again in byte-sized chunks with proper commit messages.

[apoorvajagtap]

Hello folks, we understand your perspective & concerns with the breaking commit in history, and based on the consensus reached, we have raised this draft PR that reverts the changes introduced with commit 666c197. We’ll be bumping the go version & shall add the required GHA & relevant configurations as a separate commit (in the same PR).
Please feel free to review the PR & share your feedback.

[jech]

The problematic commit has been in the repository for almost eight months now, and has still not been reverted. At this point, I find it very difficult to trust the new maintainer of gorilla/websocket, and am considering forking the repository from the last trustworthy version.

[AlexVulaj]

Hey @jech - as you can see above @apoorvajagtap opened a PR to revert the commit about a month ago. We wanted to leave it open for a small period of time in case community members had comments or discussion around the revert. We're going to go ahead and push it through.

[davidnewhall]

At this point, I find it very difficult to trust the new maintaine

Trust went out the window shortly after the project was unarchived.

our concern is that we'd lose a number of community contributions that have been made since that change

You do not lose changes by removing broken code, that's a terrible thing to even suggest. You are, however, losing users because you refuse to address this problem in a timely manner.

Good luck to all!

[TraceCarrasco]

This is open source, feel free open up a set of PRs that bring your trust back folks. The contributors here took an archived project and added support, which is very generous of them. It’s part of the open source community to support these worries - even when mistakes were made previously.

[dprotaso]

Is the plan to cut a release prior to closing this issue? It seems like the logging changes are in main ?

[jaitaiwan]

The plan is to try to clean the repo up and then issue a new release. Unfortunately this isn't the only repo in the gorilla org that we maintain and most of the repos also require our attention.

[AlexVulaj]

Hey all - some of the other maintainers have merged a new PR to revert back to commit 931041c . We've also cut a new release - v1.5.3 - that has these changes.

We understand that we handled this situation poorly - it was a learning experience for us and we regret not doing this sooner. We'll be looking to put out a more formal statement on this issue, and we appreciate all of the feedback we received - both positive and negative - from the community during this. We know we have to be better here.

For now, if anyone would mind trying the new release and letting us know if the prior observed issues are resolved, it'd be a great help to making sure this revert did everything we expect.

[jech]

Reverts to v1.5.0

$ git diff --stat v1.5.0
 .circleci/config.yml  |  6 +++---
 README.md             |  6 ------
 client.go             | 18 +++++++++++++++---
 client_server_test.go | 35 +++++++++++++++++++++++++++++++++++
 conn.go               |  8 ++++++++
 conn_test.go          | 12 +++++++++++-
 server.go             |  4 ++--
 server_test.go        |  2 +-
 util.go               | 15 +++++++++++++++
 util_test.go          | 19 +++++++++++++++++++
 10 files changed, 109 insertions(+), 16 deletions(-)

What am I missing?

[jaitaiwan]

You're not missing anything, we poorly named the commit. Go with what Alex has mentioned here... its a release with reversions for all commits after 931041c.