zeromq / jeromq

JeroMQ is a pure Java implementation of the ZeroMQ messaging library, offering high-performance asynchronous messaging for distributed or concurrent applications.
https://zeromq.org
Mozilla Public License 2.0
2.37k stars 485 forks source link

Issue with ZContext in the mtrelay guide example #40

Closed jkwatson closed 10 years ago

jkwatson commented 11 years ago

I ported the mtrelay guide example over to use the ZContext class. When I run it, I get:

Step 1 ready, signaling step 2 Exception in thread "reaper-1" java.lang.NullPointerException at zmq.Ctx.destroy_socket(Ctx.java:328) at zmq.ZObject.destroy_socket(ZObject.java:144) at zmq.SocketBase.check_destroy(SocketBase.java:942) Test successful! at zmq.SocketBase.start_reaping(SocketBase.java:758) at zmq.Reaper.process_reap(Reaper.java:133) at zmq.ZObject.process_command(ZObject.java:114) at zmq.Reaper.in_event(Reaper.java:90) at zmq.Poller.run(Poller.java:231) at java.lang.Thread.run(Thread.java:722)

I'm using the ZContext.destroy() method to clean everything up.

testcase code:

package guide;

import org.zeromq.ZContext;
import org.zeromq.ZMQ;
import org.zeromq.ZMQ.Context;
import org.zeromq.ZMQ.Socket;

/**
 * Multithreaded relay
 */
public class mtrelay_bugtestcase {

    private static class Step1 extends Thread
    {
        private ZContext context;

        private Step1 (ZContext context)
        {
            this.context = context;
        }

        @Override
        public void run(){
            //  Signal downstream to step 2
            Socket xmitter = context.createSocket(ZMQ.PAIR);
            xmitter.connect("inproc://step2");
            System.out.println ("Step 1 ready, signaling step 2");
            xmitter.send("READY", 0);
            xmitter.close ();
        }

    }
    private static class Step2 extends Thread
    {
        private ZContext context;

        private Step2 (ZContext context)
        {
            this.context = context;
        }

        @Override
        public void run(){
            //  Bind to inproc: endpoint, then start upstream thread
            Socket receiver = context.createSocket(ZMQ.PAIR);
            receiver.bind("inproc://step2");
            Thread step1 = new Step1 (context);
            step1.start();

            //  Wait for signal
            receiver.recv(0);
            receiver.close ();

            //  Connect to step3 and tell it we're ready
            Socket xmitter = context.createSocket(ZMQ.PAIR);
            xmitter.connect("inproc://step3");
            xmitter.send("READY", 0);

            xmitter.close ();
        }

    }
    public static void main (String[] args) {

        ZContext context = new ZContext();

        //  Bind to inproc: endpoint, then start upstream thread
        Socket receiver = context.createSocket(ZMQ.PAIR);
        receiver.bind("inproc://step3");

        //  Step 2 relays the signal to step 3
        Thread step2 = new Step2 (context);
        step2.start();

        //  Wait for signal
        receiver.recv(0);
        receiver.close ();

        System.out.println ("Test successful!");
        context.destroy();
    }
}
jkwatson commented 11 years ago

Ok, the issue is this. We should override the close() method on the socket that is created by the context. That method should either 1) call "destroySocket(this)" on the parent ZContext, or 2) throw UnsupportedOperationException, since the ZContext isn't removing the socket when it's getting closed.

Alternatively, ZContext should return a different class altogether when you call createSocket() on it.

miniway commented 11 years ago

If you use ZContext, you don't have to close each socket. context.destroy will close sockets for you

jkwatson commented 11 years ago

You don't have to, but nothing stops you from doing it. We should definitely make it safe to call close(), if you want to (or get rid of the method, as above).

miniway commented 11 years ago

Sorry for the confusing. Actually you must not.

Undocumented usage is

If you create Socket from low level ZMQ.Context, you must close it by yourself. If you create Socket from high level ZContext, you could close it by destroySocket otherwise ZContext.destroy will close it for you.

You can't mix it.

ZMQ.Context is low level libzmq ctz_t equivalent which is created from zmq_ctx_new. And ZContext is high level czmq zctx_t equivalent which is created from zctx_new.

ZMQ.Socket is mix of libzmq.SocketBase and czmq.zsocket_t. I think this is a design dept, but it was the legacy from initial design of jzmq. It should have created ZSocket, but it would not easy without breaking existing applications.

The zguide has an epic that describes 0MQ using low level API at chpater1 then introduces high-level czmq at chpater2. Java examples also follows the epic. So they use ZMQ.context at chapter1 examples and ZContext from chapter2.

I guess the new API is a re-designed high-level API. The examples in chapter1 might not be good candidates for a high level API. If they are written in a high level API and read within the zguide epic, stories would not fit well.

jkwatson commented 11 years ago

At the moment, however, the "higher level API" (ZContext) doesn't stop you from shooting yourself in the foot and mixing the two paradigms. I'm suggesting that we make it so you can't make the mistake of mixing the two paradigms, rather than just assume the user will know better.

I think we're in agreement. In the new API, are you suggesting we should remove the "close()" method from the Socket, rather than making it call back on the context to destroy itself? I'm happy either way, really.

miniway commented 11 years ago

I prefer Socket's "destroy" which calls context.destroySocket. And no "close". That seems to be how czmq.zsocket_t works.

jkwatson commented 11 years ago

I think that makes sense, as well. I know I'm still a complete newb, and without the full context of "high level" and "low level", I'll probably mix some stuff up. I'm just trying to design an API that makes sense, without too much regard to making it perfectly match either the "low level" or the existing CZMQ "high level" API.

Feel free to close this, if you don't think we should save the user from themselves in the "CZMQ"-style java APIs.

hintjens commented 11 years ago

I'm also happy to change the story in the guide, if we decide to completely hide the low level API. On Feb 4, 2013 4:20 AM, "John Watson" notifications@github.com wrote:

I think that makes sense, as well. I know I'm still a complete newb, and without the full context of "high level" and "low level", I'll probably mix some stuff up. I'm just trying to design an API that makes sense, without too much regard to making it perfectly match either the "low level" or the existing CZMQ "high level" API.

Feel free to close this, if you don't think we should save the user from themselves in the "CZMQ"-style java APIs.

— Reply to this email directly or view it on GitHubhttps://github.com/zeromq/jeromq/issues/40#issuecomment-13061057.

xekoukou commented 11 years ago

Does anyone use the low-api anymore? I only use poll() instead of zloop. As a newcomer, you have to learn 2 apis only to forget the first and use the second.

2013/2/4 Pieter Hintjens notifications@github.com

I'm also happy to change the story in the guide, if we decide to completely hide the low level API. On Feb 4, 2013 4:20 AM, "John Watson" notifications@github.com wrote:

I think that makes sense, as well. I know I'm still a complete newb, and without the full context of "high level" and "low level", I'll probably mix some stuff up. I'm just trying to design an API that makes sense, without too much regard to making it perfectly match either the "low level" or the existing CZMQ "high level" API.

Feel free to close this, if you don't think we should save the user from themselves in the "CZMQ"-style java APIs.

— Reply to this email directly or view it on GitHub< https://github.com/zeromq/jeromq/issues/40#issuecomment-13061057>.

— Reply to this email directly or view it on GitHubhttps://github.com/zeromq/jeromq/issues/40#issuecomment-13064055.

Sincerely yours,

 Apostolis Xekoukoulotakis
hintjens commented 11 years ago

True, and I'd like to wrap up zmq_poll too... Let's discuss this on the list. On Feb 4, 2013 7:50 AM, "Apostolis Xekoukoulotakis" < notifications@github.com> wrote:

Does anyone use the low-api anymore? I only use poll() instead of zloop. As a newcomer, you have to learn 2 apis only to forget the first and use the second.

2013/2/4 Pieter Hintjens notifications@github.com

I'm also happy to change the story in the guide, if we decide to completely hide the low level API. On Feb 4, 2013 4:20 AM, "John Watson" notifications@github.com wrote:

I think that makes sense, as well. I know I'm still a complete newb, and without the full context of "high level" and "low level", I'll probably mix some stuff up. I'm just trying to design an API that makes sense, without too much regard to making it perfectly match either the "low level" or the existing CZMQ "high level" API.

Feel free to close this, if you don't think we should save the user from themselves in the "CZMQ"-style java APIs.

— Reply to this email directly or view it on GitHub< https://github.com/zeromq/jeromq/issues/40#issuecomment-13061057>.

— Reply to this email directly or view it on GitHub< https://github.com/zeromq/jeromq/issues/40#issuecomment-13064055>.

Sincerely yours,

Apostolis Xekoukoulotakis

— Reply to this email directly or view it on GitHubhttps://github.com/zeromq/jeromq/issues/40#issuecomment-13064257.

trevorbernard commented 10 years ago

This is no longer an issue