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

ZeroMQ v6 compatibility update #111

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

hokiedsp
Copy link

This PR is to make zerorpc to work with upcoming zeroMQ v6 (currently in beta3).

  • Uses ZeroMQ v6's compatibility layer for ZeroMQ.js versions 4 and 5
  • To my limited knowledge, the only deprecation that affected zerorpc is Socket.bindSync(). The PR replaces it with bind() and returns a promise, which resolves when the socket is successfully bound.
  • Unit test files are updated to the new server bind() calls
  • All tests were successfully completed in Windows 10, Node 13.1 (see below for testing in Windows)
  • Documentation has been updated to reflect the change in bind() behavior
  • Dependent packages are all updated to the current versions
  • Updated the Buffer instantiation to avoid using the deprecated constructor

With the ZeroMQ v6 still in beta, I'm not sure if this PR should be merged at this very moment, but since ZeroMQ v5 does not build successfully under Node v13.1 I thought this is needed to be done sooner rather than later.

(Testing in Windows) Because Windows does not support ipc protocol, I had to modify random_ipc_endpoint() in test\lib\testutil.js to create a localhost tcp endpoint with random port if Windows. If there is a interest for this addition, I can create another PR for it as well.

@rolftimmermans
Copy link

rolftimmermans commented Dec 19, 2019

FWIW bindSync() has been re-added in beta 6 and should work if the deasync package is available.

I don't actually recommend using this for new projects but it certainly might make upgrading existing projects easier!

@axfelix
Copy link

axfelix commented Jun 11, 2020

This is great and badly needed to make Windows work out of the box again without having to get stuck in electron-rebuild hell on newer versions!

@bombela what's waiting to merge this?

@bombela
Copy link
Member

bombela commented Jun 11, 2020

Is ZeroMQ 6 stabilized yet?

@axfelix
Copy link

axfelix commented Jun 11, 2020

No, but it's up to beta 6, and it's been a long time waiting, there's more breaking on stable right now than there are breaking changes expected in the 6.x branch at this point...

@axfelix
Copy link

axfelix commented Jul 14, 2020

Hi! @bombela, sorry to bother you, but can you please consider merging this and bumping the dependency? It's causing a lot of trouble for our projects and older Electron has too many known vulnerabilities at this point.

@gabrielgrant
Copy link

@axfelix do you have any info about why v6 hasn't come out of beta yet?

@axfelix
Copy link

axfelix commented Jul 21, 2020

No, but I've switched to using this fork in my projects with upstream Electron, and it's working well! I think not merging this risks harming the community.

@gabrielgrant
Copy link

gabrielgrant commented Jul 23, 2020

Not sure if this is what's blocking release, but it seems v6 has an unresolved memory leak: zeromq/zeromq.js#390

This is a tough spot. On the one hand, I'd be pretty hesitant to release this as the official latest version of ZRPC and risk taking responsibility for breaking existing users' code with an upstream beta release (at least without a major version bump to ZRPC). But do agree that it would be nice to make installing this easier for new users, given it seems prior versions of ZRPC have some issues that aren't being addressed (and haven't seen attention for numerous months) as development focus appears to have (understandably) shifted to the latest version.

Thinking it may make sense to merge this into a non-master-branch, and release it as an official ZRPC beta? How would that work for you, @axfelix and @bombela ?

@rolftimmermans any recommendations on how best to proceed, or insight into what still needs to be done before an official ZMQ v6 release?

@bombela
Copy link
Member

bombela commented Jul 23, 2020 via email

@hokiedsp
Copy link
Author

I'm glad this PR is being talked over right now as I'm just about to get back into using it in my project :-)

Anyway, reading @gabrielgrant's comment:

risk taking responsibility for breaking existing users' code

made me looking back at what I've done. Would it be better to create a new function bindAsync() and keep the original bind() intact as long as ZMQ supports it? And steer new users (like myself) to use bindAsync() instead.

This means introducing socket.bindAsync() and server.bindAsync().

@axfelix
Copy link

axfelix commented Jul 23, 2020

The vast, vast majority of js libs have gone in the opposite direction lately -- making async behaviour the default and defining a separate sync() function that's compatible with the legacy implementation.

@hokiedsp
Copy link
Author

hokiedsp commented Jul 23, 2020 via email

@Apollon77
Copy link

I was also thinking to use this in a project. Is there any progress or next steps planned? ;-)

@bombela
Copy link
Member

bombela commented Mar 28, 2021 via email

@gabrielgrant
Copy link

Unfortunate to see that ZMQ not only seems to have not made any progress on this v6 memory leak regression in the past year, but doesn't appear to have seen any development in almost that long...

A couple people have asked about the status in their issue tracker recently:

@gabrielgrant
Copy link

gabrielgrant commented May 23, 2021

@hokiedsp I think you've rightly identified two possible paths:

  1. Maintain backwards compat with a minor version release. In this case continuing usage of the deprecated function and adding a bindAsync() method makes sense.
  2. Break backwards compat, and update to the modern async/promise-style APIs throughout

It seems to me that either of these could make sense. I guess best case would be to have both options (major update for usage in new projects; minor update to easing the transition in existing projects). But unless you want to do that duplicate work, my preference would probably be to just fully move on up to the modern APIs

Did you ever get a chance to take a stab at updating other methods to the async/promisified versions?

@hokiedsp
Copy link
Author

@gabrielgrant -

Did you ever get a chance to take a stab at updating other methods to the async/promisified versions?

No I didn't. I've been off to Python side of the things lately and I'm crossing my fingers that an updated version of ZeroMQ will be out before I get back to JS matters.

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.

6 participants