search

0rpc / zerorpc-node

zerorpc for node.js
http://www.zerorpc.io
Other
705 stars 167 forks source link

ZeroMQ v6 compatibility update #111

Open hokiedsp opened 4 years ago

hokiedsp commented 4 years ago

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

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Is ZeroMQ 6 stabilized yet?

axfelix commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

axfelix commented 4 years ago

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 commented 4 years ago

Not sure if this is what's blocking release, but it seems v6 has an unresolved memory leak: https://github.com/zeromq/zeromq.js/issues/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 commented 4 years ago

A beta branch sounds very reasonable. It might be possible to set the version on npm carefully enough, such that the confusion is minimal.

I will look into it when I get some time.

On Thu, Jul 23, 2020 at 11:03 AM Gabriel Grant notifications@github.com wrote:

Not sure if this is what's blocking release, but it seems v6 has an unresolved memory leak: zeromq/zeromq.js#390 https://github.com/zeromq/zeromq.js/issues/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). 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).

Thinking is 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 https://github.com/axfelix and @bombela https://github.com/bombela ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/0rpc/zerorpc-node/pull/111#issuecomment-663151276, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABUZDDPUEYIAX7JOWIYZNLR5B3O5ANCNFSM4JLQJALQ .

hokiedsp commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

I do agree with that, but I was concerned about the backward compatibility.  Also, this PR only addresses bind() which turned out to be the only deprecated zmq sync function relevant to zerorpc. If backward compatibility is a non-issue, wouldn’t it better to switch over all sync/callback-based functions to async/promise functions that zmq v6 (newly?) implements? I’d be glad to take a stab at it, but it might not happen for a little while.

Apollon77 commented 3 years ago

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

bombela commented 3 years ago

I will take a look in the next few weeks.

On Sun, 28 Mar 2021, 05:29 Ingo Fischer, @.***> wrote:

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

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/0rpc/zerorpc-node/pull/111#issuecomment-808890242, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABUZDDBX2XPES4SDGCXNELTF4VKLANCNFSM4JLQJALQ .

gabrielgrant commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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.