Delve: service/rpc: RPC server should serve requests consequently

[zolotov]

Please answer the following before submitting your issue:

Note: Please include any substantial examples (debug session output,
stacktraces, etc) as linked gists.

1. What version of Delve are you using (dlv version)? 82ece54
2. What version of Go are you using? (go version)? 1.6.2
3. What operating system and processor architecture are you using? osx/windows
4. What did you do? use rpc protocol for debugging
5. What did you expect to see? consequently invoked procedures
6. What did you see instead? createBreakpoint and continue methods are reordered randomly

rpc.ServeCodec runs all remote methods via go-routine. So it cannot give any guarantees about methods invokation order. As a result we have following bugs in IDEA<->Delve integration when breakpoints are set after continue command invokation: go-lang-plugin-org/go-lang-idea-plugin#2360

I've suggested the https://github.com/derekparker/delve/pull/520 which uses ServeRequest method instead, which is synchronously approach of serving request. It fixes the problem but unfortunately it's not a proper implementation because it breaks halting the process.

[aarzilli]

I've been looking into this and I don't think we can do it without rewriting net/rpc, it can be used in one of two ways:

1. either it executes each command in its own goroutine (which means there's nothing we can do to ensure that the order we execute requests is the same as the order we received them)
2. or it waits until each command is complete before reading a new one (which means a next or continue request can not be interrupted).

If I missed something obvious I would like to hear about it. As things stands, AFAIAC, this behavior isn't going to change anytime soon. Is there really no way in IDEA to create a synchronization barrier between all the breakpoint setting and the call to continue?

[aarzilli]

For example, if you added this:

private static class WaitGroup {

    private int jobs = 0;

    public synchronized void add(int i) {
        jobs += i;
    }

    public synchronized void done() {
        if (--jobs == 0) {
            notifyAll();
        }
    }

    public synchronized void await() throws InterruptedException {
        while (jobs > 0) {
            wait();
        }
    }

}

private WaitGroup outstandingBreakpoints = new WaitGroup();

to DlvDebugProcess, then added outstandingBreakpoints.Add(1) to DlvDebugProcess.MyBreakpointHandler.registerBreakpoint before send, and a outstandingBreakpoinst.done() to done and rejected in the same method. Finally add a outstandingBreakpoints.await() to DlvDebugProcess.command. Why wouldn't that be ok?

[zolotov]

@aarzilli I got your point. Ok, we'll make synchronization on our side. Thank you for looking into this

[ignatov]

@aarzilli @derekparker Does that mean that other RCP clients should implement their own synchronization?

[aarzilli]

@ignatov it means that if a client is sending two requests A and B and B arrives before we responded to A then B may be executed before A. If the client wants B to be executed before A the client should wait for the response to A to be received, whether or not this implies some sort of synchronization in the client depends on how the client is implemented.

[dlsniper]

Could we have something in delve which basically forces waiting (or delaying for lack of a better term) on executing commands out of order until they are correctly received? Something like: if Delve receives B but not A, then it waits on processing B until the "current execution" can be set to "B". A further version of this could say if "A" arrives in a timely fashion (say ~1-2 seconds?) then process "A", else "advance" to "B".

[aarzilli]

@dlsniper: not that I can see.

[derekparker]

@aarzilli maybe we have an internal command loop, taking commands via a channel. Any command that would modify the state of the process is put into a channel and handled sequentially. Halt would be an exception, because it essentially just sends out a signal and sets an internal flag to stop a blocking wait call.

[aarzilli]

If we ditch net/rpc we can call directly ReadRequest in jsonrpc and write our own dispatch loop that preserves ordering.

[evantorrie]

@aarzilli I agree that you probably need to ditch net/rpc. When looking at the implementations of gdb and lldb client/server, they both explicitly allow for NOTIFY style packets that are not part of a request/response model.

In addition, it appears that all explicit debugger commands from the client are executed synchronously on the server and the client waits for a response, _except_ for the case of an interrupt or halt from the client (which can be sent out of band). See https://github.com/llvm-mirror/lldb/blob/master/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp#L1372 and https://github.com/llvm-mirror/lldb/blob/master/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp#L1416 for examples of how lldb handles this (it emulates the gdb remote server protocol for the most part).

The standard JSONRPC model of request/response with no guarantee of ordering of request execution seems incongruent with the model that both gdb and lldb use.