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

when gateway.close is called, the pinger should be terminated #9

Closed
wants to merge 35 commits into from

Conversation

cameronelliott
Copy link
Collaborator

No description provided.

@nustiueudinastea
Copy link
Contributor

Hey @cameronelliott, I will be maintaining a separate fork from now on here: https://github.com/Apollo-Systems/janus-go

Doing this because things are moving very slow on this repo and this lib needs all the help it can get. If you wish to do so, add your PR on that repo.

@cameronelliott
Copy link
Collaborator Author

cameronelliott commented Dec 15, 2020

@nustiueudinastea Good idea!
Yea, the author is good programmer, but I know how we all get busy with other projects, I am just so glad he created this and shared it.
I am also going the same route, and have a fork here:
https://github.com/cameronelliott/janus-go

Hopefully you and I, and Notedit if he has time and interest, can compare notes after some work is done and figure out how to combine the best parts to get the changes we all want.
😄

The main changes I am making are:

  • Switch to nhooyr/websocket for websockets, the big win here IMHO is context.Context support which better supports structured concurrency which in theory can help concurrency robustness when done well.
  • Hope to remove goroutine creation in the library, I usually prefer to do this as a library's caller, as it makes it easier for me to think/reason about thread/goroutine issues
  • Hope to remove chan creation in the library, again I like to create these externally to libraries I use if possible, it can make thinking/reasoning about robustness easier for me.

I'm just getting started, hopefully when I get farther, you can take a look at my changes and see if you think they are useful, and you can suggest other stuff you think would help, and maybe we can merge at some point.

@nustiueudinastea
Copy link
Contributor

@cameronelliott those changes sound very useful. Good characteristics for a library! Will keep an eye on your repo

@cameronelliott
Copy link
Collaborator Author

After some thought I reopening this so you can get notifications. !

@notedit
Copy link
Owner

notedit commented Dec 16, 2020

@nustiueudinastea I did not use this lib any more, I will add a link for your forking.

@notedit
Copy link
Owner

notedit commented Dec 22, 2020

@cameronelliott I add you as a maintainer。

@cameronelliott
Copy link
Collaborator Author

@notedit
Thank you for adding me as a collaborator.

Do I have your permission to add a link to my repo and to document and to handle most issues there?

Are there any other guidelines or input you have?

My perception is that you are busy with other projects, and you really just want someone to take this over.

But I just think it's good to clarify with you.

So I'm asking for permission to add a link to my repo, and add some commentary to the readme about the plans for this project, and the plans for the two projects respectively.
And how they might differ as time goes forward. (although I might just push all changes to this repo also if that seems to develop as a good idea)

Do I have your permission to do that?
That way issues will not come up on your notification board.
If that's not what you want, please tell me.

Thank you again for adding me.
Hopefully I can make you happy with the outcomes!
😊

@notedit
Copy link
Owner

notedit commented Dec 22, 2020

@cameronelliott ok you can add a link for your fork and do what you want :)

@cameronelliott
Copy link
Collaborator Author

For the moment, rather than merge this PR into this repo, I have decided to keep the numerous changes separate.
You can explore this repo to see if it better suits your needs:
https://github.com/cameronelliott/janus-go

The main goal is to get to a high level production usability.

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.

None yet

4 participants