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

c.sess.Send error handling #291

Open
KSDaemon opened this issue Jan 5, 2023 · 3 comments
Open

c.sess.Send error handling #291

KSDaemon opened this issue Jan 5, 2023 · 3 comments

Comments

@KSDaemon
Copy link
Collaborator

KSDaemon commented Jan 5, 2023

Right now all over the sources, we do not check and process the return error value from c.sess.Send call.

Golang linter of course is complaining about that.

  Error: Error return value of `c.sess.Send` is not checked (errcheck)
  Error: Error return value of `c.sess.Peer.Send` is not checked (errcheck)
  Error: Error return value of `c.sess.Send` is not checked (errcheck)
  Error: Error return value of `c.sess.Send` is not checked (errcheck)
  Error: Error return value of `c.sess.Send` is not checked (errcheck)
  Error: Error return value of `c.sess.Peer.Send` is not checked (errcheck)
  Error: Error return value of `c.sess.Peer.Send` is not checked (errcheck)
  Error: Error return value of `c.sess.SendCtx` is not checked (errcheck)
  Error: Error return value of `c.sess.SendCtx` is not checked (errcheck)
  Error: Error return value of `c.sess.SendCtx` is not checked (errcheck)

We need to agree on what to do:

  • Just mark such places as ignored by linter
  • Or really check for the error. But then we'll get another question: what to do if there is an error because if that is a really failure — the underlying connection will be closed and there is no need to process the error somehow
@KSDaemon
Copy link
Collaborator Author

KSDaemon commented Jan 5, 2023

@gammazero What do you think?

@gammazero
Copy link
Owner

gammazero commented Sep 8, 2023

The error response from Send is not used, and the error returned by TrySend is only used in a couple of places, where it is questionable that it is even necessary. There really is no need for Send to return an error.

I think the best solution is to change Send to return a write-only channel. That way the sender can choose how to handle the write. The sender can block, can select with other channels, etc. This also gets rid of the TrySend and SendCtx functions and simplifies APIs. Unfortunately, this constitutes a breaking API change and would require a major revision, to follow semver strictly. WDYT?

However, this might not be considered a breaking change if Send is considered an internal API used when writing your own router or client implementation using the wamp package. You can see this is the case since all of the examples do not need to change

If we make this change to Send, would it be worth a major revision update? Should we consider forking the repo to create a new project where we can restart the versioning before 1.0.0 and make many other API and structure changes?

@KSDaemon
Copy link
Collaborator Author

KSDaemon commented Sep 9, 2023

I think it is ok. We follow SemVer, right? - Right. So I don't see any problem in sometimes breaking changes if that simplifies the logic and improves the project maintainability and increase the project quality in general. All we have to do: introduce the changes, describe the migration path (v3 → v4) so it will be clear what was changed and how to adopt the new approach and bump the major version.

Regarding forking - I don't think it is needed. We're not a big team just a few enthusiasts so let's keep it convenient for us. We can accumulate all the changes in separate branch, then start publishing the v4.pre, v4.beta and so on while polishing the changes. I don't think we have enough power to support more than one version like v3 and the next one at once. So whenever we release the next major update - we'll probably stop supporting the previous one and will encourage our users to update.

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

No branches or pull requests

2 participants