WIP: Refactor to HTTP client/server model

NOTE This is a work-in-progress, and the PR is to facilitate ongoing design discussion. This PR is intended to address #22.
Goals

Introduce an HTTP server using net.Listener to provide a JSON-based remote API to the debugger
Support RESTful operations for state retrieval and commands
Support a long-polling watch requests to support event notifications
Provide a Go client interface and default HTTP implementation
Refactor the terminal to be a pure API client consumer

The original PR is based on this comment on #22.

Thank you for taking the time to put together an initial POC PR. I definitely like where this is headed, in particular I am a fan on the versioned API system for the benefits you mentioned, I think that is a win.
I'd like to spend some time discussing the design of the server. I want to avoid an actual http server implementation. I think the server should remain lightweight, and should be able to communicate over basic sockets (unix, tcp or websocket). The advantage we get there is all of the above satisfy the net.Conn interface, so the client/server can be agnostic of socket type. We could even go one step further for the base case (using dlv from the command line) and implement something that satisfies the net.Conn interface but sends data over a channel, since both client / server will be in the same process.
From there, we can use that connection to send JSON requests / responses.
{
    "command": "break",
    "args": "example.go:23",
    ...
}

The server can parse the JSON, dispatch based on command and send back a JSON reply. On the client side, there should be an abstraction that hides the encoding / decoding.
I think this refactor is a great idea, and I'm open to discussion on implementation.

I am in the middle of a socket server / command / event implementation as we speak. I'll get a commit up for discussion very soon and we can see how far off we are from what you just described.

@derekparker
Still working through some weird bug I somehow introduced[1], but I think you can probably now assess whether I'm headed in the same direction your brain was. Basically, this is now an async event model/state reconciler, where the Terminal uses a client to send commands, and receives state updates from the debugger which the Terminal uses to maintain its own internal cached view of the process state (see breakpoints and threads).
I'm using event/command type dispatch on the backend.

I'd like to spend some time discussing the design of the server. I want to avoid an actual http server implementation. I think the server should remain lightweight, and should be able to communicate over basic sockets (unix, tcp or websocket)

Agree. I'm using a concrete websocket server at the moment, but I can already see how a bit more abstraction would allow us to have a "service" layer that does all the API command handling and event notification with DebuggedProcess while pushing the actual event publishing/reading into messaging implementations (e.g. websocket) somewhere else.
[1] proc.BreakByLocation is returning could not set hardware breakpoint: no such process when fed what seems to be a valid location. Still debugging. Maybe I should run delve with delve...

Are there any issues I should be aware of when using DebuggedProcess across thread boundaries?

Yes, DebuggedProcess makes use of ptrace, and all ptrace commands must be
run from the same thread that attached to the other process.
On Saturday, March 21, 2015, Dan Mace<EMAIL_ADDRESS>wrote:

Are there any issues I should be aware of when using DebuggedProcess
across thread boundaries?
—
Reply to this email directly or view it on GitHub
https://github.com/derekparker/delve/pull/88#issuecomment-84257690.

Yes, DebuggedProcess makes use of ptrace, and all ptrace commands must be run from the same thread that attached to the other process.

lol. That certainly explains some things.. thanks!

Some progress in the latest commit:

Instead of working with the DebuggedProcess directly (which isn't thread safe), the server implementation is provided with a ProcessManager which allows the server to queue a function on a channel. The function gets access to the DebuggedProcess when the function is eventually executed in the main thread context (which owns the DebuggedProcess). The ProcessManager also provides synchronous call semantics as a convenience.
The Terminal now has access only to client.Interface, and I'm implementing functions piece by piece.

As is usually the case, this is turning out to be way more work than expected, but I think it's progressing pretty quickly. API consensus, testing, error handling, logging, docs, and polish will likely take up a significant amount of time once the actual architecture has settled. I'm willing to write down an updated TODO to capture all the remaining requirements.
I'm happy to keep plugging away at this if you feel like it's still headed where you want to be. If not, please feel free to let me know, I won't take it personally. This has been fun regardless, and I appreciate the feedback!

I really like where this is heading.
I think we can get away with having a dedicated "ptrace" thread instead of a dedicated "DebuggedProcess" thread, because really it's only the ptrace calls that have to be pinned to a thread via runtime.LockOSThread. Thoughts?

Just my opinion but ptrace is a *nix implementation detail - you might want
to keep DebuggedProcess for other platforms.
On 21 March 2015 at 15:25, Derek Parker<EMAIL_ADDRESS>wrote:

I really like where this is heading.
I think we can get away with having a dedicated "ptrace" thread instead of
a dedicated "DebuggedProcess" thread, because really it's only the ptrace
calls that have to be pinned to a thread via runtime.LockOSThread.
Thoughts?
—
Reply to this email directly or view it on GitHub
https://github.com/derekparker/delve/pull/88#issuecomment-84431860.

@pnasrat definitely true, which is why I think I'd like a ptrace only thread for systems that leverage ptrace, instead of forcing DebuggedProcess to be locked to a thread when it is not necessary.
That way, non-*nix systems will not start the ptrace thread and will never have to interface with that portion of the code.

Yeah that makes sense thanks for the clarification.
On 21 March 2015 at 15:32, Derek Parker<EMAIL_ADDRESS>wrote:

@pnasrat https://github.com/pnasrat definitely true, which is why I
think I'd like a ptrace only thread for systems that leverage ptrace,
instead of forcing DebuggedProcess to be locked to a thread when it is not
necessary.
That way, non-*nix systems will not start the ptrace thread and will never
have to interface with that portion of the code.
—
Reply to this email directly or view it on GitHub
https://github.com/derekparker/delve/pull/88#issuecomment-84434012.

Good thoughts re: ptrace. Next time I sit down with this I'll think about it in more detail. Immediately, I might keep pushing forward with the existing abstraction which locks up DebuggedProcess just so I can stabilize the whole program/finish the client, and then figure out how to refine the process interface in terms of ptrace.

That sounds good to me.
Just a heads up, the last commit on master https://github.com/derekparker/delve/commit/684dc92ccd1775fcef90bca42110caf86615ff05 may effect you, however the previous commits should not. I wanted to get that last commit in to handle natural death better on OS X, and these changes should be brought over as part of the refactor.
Moving forward I'm going to freeze changes that may effect this refactor until it's merged in, and work on some of the core proctl stuff.

Sweet, thanks for the heads up. I'll rebase.

On Mar 21, 2015, at 6:36 PM, Derek Parker<EMAIL_ADDRESS>wrote:
That sounds good to me.
Just a heads up, the last commit on master 684dc92 may effect you, however the previous commits should not. I wanted to get that last commit in to handle natural death better on OS X, and these changes should be brought over as part of the refactor.
Moving forward I'm going to freeze changes that may effect this refactor until it's merged in, and work on some of the core proctl stuff.
—
Reply to this email directly or view it on GitHub.

@derekparker
I figured out how to better factor things in a way which makes things much simpler (I hope).
There's now a Debugger in the proctl/server package which knows how to create and provide safe access to a DebuggedProcess via API objects on channels. The WebsocketServer is now a really simple event pump which knows how to feed API commands from a client to a Debugger and dispatch events from the Debugger back to clients. This has at least two big benefits:

The thread pinning behavior is internalized to the Debugger server. Just create a Debugger, run it in a goroutine, and start interacting with it safely via channels. Debugger has no need for weird process manager indirection, since it's internally single-threaded.
The Debugger stands alone and interaction is entirely via API objects over channels, so WebsocketServer becomes just an implementation of an event pump; we could now very easily implement a server endpoint with UNIX sockets (or whatever).

Still slowly working through the remaining client API calls. There's still a lot of work to do as we go to polish off the rough edges with the UX in the terminal. I've also been chipping away at solving the various issues with shutting down all these moving parts. It's better than before, but still not done.

A few more things added:

Started hardening the shutdown logic across the board; still working through exit detection on the server. The server is still mis-reporting exited status if you break, continue, and let the program end naturally. I did quite a bit of Terminal refactoring to get everything coming down gracefully.
Added glog for server side logging. glog's default flush interval is annoyingly slow, so I've been running with --logtostderr=true.
Changed NextEvent in the client to instead be Events which returns a channel. Calling Close on the client shuts down the associated channel writer.

@derekparker
I realize this PR is becoming absolutely massive. I'm not sure if there's a good way to break up smaller chunks to incrementally merge in. I can keep rebasing and carrying it all the way until we're at a point where the both of us can work off the same branch to take it the last mile, if you're okay with that. Just let me know if you want any logistical changes.

@ironcladlou I'll be able to take a closer look in a few hours, but I do have a suggestion for trimming this PR up a bit.
How about we keep this initial PR to a pure refactor, in other words do not introduce the concept of a websocket server, unix socket server, etc yet. Instead, let's focus on getting to a pure client/server mode of operation for the base CLI (using a net.Conn compatible channel wrapping abstraction) and then get that merged. From there, we should have standardized how communication with the server should happen and it should be relatively trivial to add in a websocket server, unix socket server, etc... Essentially, finish out the spirit of the refactor and dogfood it with the CLI that's already in place. From there build out other clients as necessary.

@derekparker Unless I'm misinterpreting your words, I've already provided what you're asking; we'll re-sync once you've had a chance to go over the current branch. Thanks!

Yeah I see that channel / event abstraction stuff in this PR, I mostly meant ripping out the websocket stuff (just for this PR) and submitting that separately, does that make sense?

@derekparker

Yeah I see that channel / event abstraction stuff in this PR, I mostly meant ripping out the websocket stuff (just for this PR) and submitting that separately, does that make sense?

Ah! Yeah, good idea. I can stash the websocket stuff and make the default client implementation a simple channel based thing. That will reduce the scope of the PR and also make developing this a bit simpler (and will solve shutdown issues w/ http servers I didn't want to deal with yet).

@derekparker Ripped out the websocket stuff and replaced with a local channel client. Shutting down seems much more stable now. Also fixed up the process API model and corrected terminal shutdown handling with regards to detach/kill/etc.
This more like what you had in mind?

This is getting really close. I accidentally misled you a bit as I meant to say io.ReadWriteCloser instead of net.Conn.
I have some ideas for LocalClient and Debugger. I think NewDebugger should look something like this: NewDebugger(conn io.ReadWriteCloser) and LocalClient should strictly adhere to io.ReadWriteCloser since any connection type, be it local, unix, tcp, websocket should all respond to those.
I think LocalClient should be split up into 2 things, LocalConn and LocalClient. LocalConn implements only io.ReadWriteCloser and LocalClient takes in a LocalConn and uses it to serialize commands into JSON and pass it through the channel (using only Write) i.e. LocalClient will use LocalConn when initializing a json.Encoder.
That way, LocalConn can implement only those 3 methods by simply writing the byte slice to the channel, and internally the chan will be chan []byte. That makes things nice because we can pass that client into Debugger and it can "read" from that client for event dispatching, and in the future we can pass in a websocket/unix/tcp connection and it'll be all the same.
So LocalConn will be something like:
func NewLocalConn() *LocalConn {
        return &LocalConn{localChan: make(chan []byte)}
}

func (lc *LocalConn) Write(cmd []byte) (int, error) {
        lc.localChan <- cmd
        return len(cmd), nil
}

func (lc *LocalConn) Read() ([]byte, error) {
        return <-lc.localChan, nil
}

Then the server run loop will essentially look like:
func NewDebugger(conn io.ReadWriteCloser) *Debugger {
        ...
        debugger := &Debugger{
                commandDecoder: json.NewDecoder(conn),
                ...,
        }
        ...
}

func (d *Debugger) Run() error {
        var command DebugCommand // or whatever we name it
        for {
                err := d.commandDecoder.Decode(&command)
                // handle err
                // dispatch command
        }
}

Does this makes sense? I hope I'm explaining it well enough.

By the way, @ironcladlou thanks again for hacking on this, I know this is a large refactor and I appreciate you taking it on.

I think I see what you mean; and it's a really cool idea. What you're proposing seems way more flexible.
Only my next commit can reveal whether I truly understand. :wink:

By the way, @ironcladlou thanks again for hacking on this, I know this is a large refactor and I appreciate you taking it on.

My pleasure! This is fun, and I've learned a ton already. Thanks for being so responsive.

@derekparker
(When I say "connection" here, I mean an io.ReadWriteCloser)
Actually, a question arose as I got deep into this. The current server design relies on bidirectional communication. How can a bidirectional system be represented using a shared connection backed by a single channel? These are the actors (currently in the same process):

Server (reads commands, writes events)
Terminal (reads events, writes commands)

This is currently facilitated by the Debugger exposing a separate Events and Commands to client/server implementations who perform duplexing (e.g. via channel exchange, websockets, etc.)
In your proposed model, how many connection instances are involved, and how do they become related in such a way that both the client and the server can perform bidirectional communication? Is there something that obviates the need that I'm just missing?
At one point made a bidirectional connection using an in/out channel internally, which seems correct, but then I couldn't decide what the right semantics would be for actually linking it with another connection. For example, does that mean both Terminal and Debugger get connections? If so, how are those connections actually linked when they're defined (or after)?
Basically I'm feeling a bit dense. Would you mind adding a little bit more detail to the design you're proposing? That might be helpful. These networking patterns are relatively new to me. Thanks!

So I'm not 100% sure what the problem is here, channels are already bi-directional. So the local conn object needs only a single channel. And since we're sending JSON over, it should have a type of []byte.
The way I see it, the debug server should be able to accept any io.ReadWriteCloser type and do dispatch. In the local case, both the server and the client should receive the same local connection object. When we move to unix/tcp/websocket connection, we will still initialize the debug server with some sort of connection, it shouldn't actually matter since they will all satisfy the proper interface.
Both client and server should be initialized with their proper connection type. As of right now, the only client we will be initializing will be our CLI client. Maybe in the future we will have our own web ui which will open a websocket connection via JS or something. IDE plugins, etc.. will connect on their own to the socket that the debug server binds to.
Does this make sense? If it's still unclear, or if I am totally missing something with this design, we should hop on IRC or something and work through it. I am traveling later this evening for a conference but I should be available tonight or tomorrow.

@derekparker
Here's an example:
http://play.golang.org/p/zP0CI5VTGy
Channels are bidirectional, but in order to implement a full-duplex connecton, a single channel won't suffice if the connection is shared by both peers. You end up with what I demonstate in the playground. The client and server both get the chance to receive their own messages they just wrote to the channel, since they're both reading and writing from the same channel. As you can see, the client receives commands destined for the server, and the server receives events destined for the client. Then, when both sides of the connection stop writing, deadlock occurs.
Don't you need a pair of connections implemented with an in/out channel pair? The connections would then need wired somehow to read and write from each other's opposite Read/Write methods.
I still fully admit the possibility that I'm just missing something. cc @pmorie who reviewed this problem with me.

Ah, yeah I see the problem now. You're definitely right, we'll need a pair
of channels. Let me think through that for a bit to try and find a clean
solution. Definitely post anything here if you have a revelation.
On Tuesday, March 24, 2015, Dan Mace<EMAIL_ADDRESS>wrote:

@derekparker https://github.com/derekparker
Here's an example:
http://play.golang.org/p/zP0CI5VTGy
Channels are bidirectional, but in order to implement a full-duplex
connecton, a single channel won't suffice if the connection is shared by
both peers. You end up with what I demonstate in the playground. The client
and server both get the chance to receive their own messages they just
wrote to the channel, since they're both reading and writing from the same
channel. As you can see, the client receives commands destined for the
server, and the server receives events destined for the client. Then, when
both sides of the connection stop writing, deadlock occurs.
Don't you need a pair of connections implemented with an in/out channel
pair? The connections would then need wired somehow to read and write from
each other's opposite Read/Write methods.
I still fully admit the possibility that I'm just missing something. cc
@pmorie https://github.com/pmorie who reviewed this problem with me.
—
Reply to this email directly or view it on GitHub
https://github.com/derekparker/delve/pull/88#issuecomment-85603996.

How about:

Use net.Listen to bootstrap a tcp listener bound to localhost:0 (random port), which provides the server with client net.Connection
Use net.Dial aimed at the server net.Listener, which provides the client with a server net.Connection?

I don't think we'd even need a custom connection type then. We could later move the encoding/decoding stuff into a connection wrapper or something.

P.S.,

we should hop on IRC or something and work through it

I'm danmace on FreeNode and am there all day throughout the work week, EST.

Another thought: If the Debugger accepted a net.Listener rather than a connection/io.ReadWriteCloser, then it could initialize but then wait for a client connection before launching the process. This would give the client the opportunity to receive events related to the actual debugger process bootstrapping. Like a "wait for client attach" mode.
I don't think it reduces the flexibility (still uses an io.ReadWriteCloser for communication), and takes advantage of the fact that the Debugger really is a server and so can have server-like semantics  (as opposed to being a "peer" using only a connection).

This also might be interesting: https://github.com/docker/libchan. I haven't taken an extremely close look, but this standardizes communication using any kind of medium, including channel, tcp/unix socket, websocket.

Was talking with @ncdc about this, who does a ton of work with Docker upstream and on protocol work. He has some opinions about libchan and also suggested checking out streams and gotalk. I've asked if he'd mind chiming in here.
My current thinking is that given the variety of clients to this server in terms of IDE hosts (vim plugins, java plugins, python plugins), we might want something that at least allows plugging in websockets (which could rule out streams).
I'm all for the idea of abstracting away the transport stuff so that we can work with channels one way or another.

I really didn't mean to turn this into a Red Hat pile-on; hobby and dayjob worlds are colliding. :relaxed:

'Oops let me tag like 5 more Red Hat people in' --@ironcladlou
On Tue, Mar 24, 2015 at 4:31 PM Dan Mace<EMAIL_ADDRESS>wrote:

I really didn't mean to turn this into a Red Hat pile-on; hobby and dayjob
worlds are colliding. [image: :relaxed:]
—
Reply to this email directly or view it on GitHub
https://github.com/derekparker/delve/pull/88#issuecomment-85680529.

I only have two left who care this much about debuggers, and I'm saving them for a special occasion

On Mar 24, 2015, at 8:36 PM, Paul Morie<EMAIL_ADDRESS>wrote:
'Oops let me tag like 5 more Red Hat people in' --@ironcladlou
On Tue, Mar 24, 2015 at 4:31 PM Dan Mace<EMAIL_ADDRESS>wrote:

I really didn't mean to turn this into a Red Hat pile-on; hobby and dayjob
worlds are colliding. [image: :relaxed:]
—
Reply to this email directly or view it on GitHub
https://github.com/derekparker/delve/pull/88#issuecomment-85680529.

—
Reply to this email directly or view it on GitHub.

I'm going to play with a libchan implementation to see how it looks. For science!

@derekparker Using the net.Conn model, with the capabiities libchan provides, now I'm also wondering what the actual client/server interaction model should be. Right now it's 100% disassociated asynchronous events- e.g., add a breakoint, server reports status as an async message not associated with the request.
What you're suggesting provides associated request/response semantics. Would it make for a more useful Go interface if the client methods used this flexibility to present a blend of RPC and event semantics to the user? For example:

client.AddBreakPoint() would be synchronous; it blocks until the server actually reported an outcome for that command, which the client would return.
client.Threads() would return the current thread list synchronously.
client.WatchThreads() would return a channel that receives api.Thread updates via a goroutine the Debugger created when handling the client request.

We could implement Watch type variants for any interesting proctl state.

Thanks for taking the time to explain your ideas for this stuff. I think I'm starting to better understand the power that comes with the patterns you're proposing. Please correct me if I'm off base.

Also, if we choose libchan, clients will require SPDY, and we'll have to track https://github.com/docker/libchan/issues/84 for HTTP/2 support which also implies refactoring to be compatible with dmcgowan/streams at some point.
What are the implications to external client implementations (e.g. as used by IDE plugin authors) of each choice? I think I'd like get a bit more clarity and consensus before further refactoring.

I think ideally I'd like to get by without adding an external dependency if
at all possible. If we can come up with a clever way to wrap the channel
stuff for local client I think we'd be there. Failing that, maybe use
sockets like you previously suggested.
On Tuesday, March 24, 2015, Dan Mace<EMAIL_ADDRESS>wrote:

Also, if we choose libchan, clients will require SPDY, and we'll have to
track docker/libchan#84 https://github.com/docker/libchan/issues/84 for
HTTP/2 support which also implies refactoring to be compatible with
dmcgowan/streams at some point.
What are the implications to external client implementations (e.g. as used
by IDE plugin authors) of each choice? I think I'd like get a bit more
clarity and consensus before further refactoring.
—
Reply to this email directly or view it on GitHub
https://github.com/derekparker/delve/pull/88#issuecomment-85786657.

I think ideally I'd like to get by without adding an external dependency if at all possible. If we can come up with a clever way to wrap the channel stuff for local client I think we'd be there.

My concern is that neither io.ReadWriteCloser or net.Conn provide request/response semantics by themselves. We still need a protocol and a convenient library wrapping that protocol and providing us with request/response semantics, and that's a lot of work that's already being solved elsewhere. What I have in the branch now doesn't provide a nice experience compared to the revised API I described here.
To achieve the desirable request/response semantics, the feature set we'd end up implementing would eventually want to look like libchan & friends. It seems pragmatic to just adopt an decent existing abstraction that faciliates the model we would end up trying to invent anyway, provided the protocols the abstraction supports are readily available to client authors.
Can we asess whether there's community support/convergance on SPDY and soon HTTP/2? @ncdc might have some insights there. If they are widely supported by the target client audience, it would greatly simplify the client/server code if we could lean on something like libchan. For example, I'm not seeing any Python standard library support. JavaScript?

Failing that, maybe use sockets like you previously suggested.

We could get the semantics we want with a WebSocket implementation today and have confidence that client support will exist. We'd still want something like libchan's interfaces which would just be implemented with WebSockets. We'd still want to use a third-party library (like gorilla) in this case.
I guess in summary, I'm proposing we choose between libchan or custom interfaces+websockets for the sake of both API user experience and internal implementation simplicity.
Thoughts?

Err, we sockets don't provide correlated request/reply semantics, so I guess those don't really help since we'd have to do routing/correlation ourselves.

@derekparker I think I'll take you up on that offer to hash out the remaining discussion on IRC. We can post a summary of the outcome here.

Sounds good. I've started a delve channel on irc.freenode.net so feel free
to hop on. I'll be on shortly.
On Wednesday, March 25, 2015, Dan Mace<EMAIL_ADDRESS>wrote:

@derekparker https://github.com/derekparker I think I'll take you up on
that offer to hash out the remaining discussion on IRC. We can post a
summary of the outcome here.
—
Reply to this email directly or view it on GitHub
https://github.com/derekparker/delve/pull/88#issuecomment-86006482.

The audience for a Go debugger server interface (e.g. editor/IDE integrators) consists of users employing a broad variety of technologies (Go, Python, Java, Emacs Lisp, VimScript, shell, the list probably goes on.) In the end, we decided to weigh client compatibility/friendliness as most important. The more tools that can easily consume the API, the better.
After much discussion with @derekparker on IRC, we reached the following conclusions:

We do not want to write a new protocol, and need request/response semantics.
Although solutions such as libchan and gotalk provide very nice server side interfaces for Go, they:
Impose a requirement that clients understand a custom protocol with limited or nonexistent support from standard libraries or the community
Currently require transport protocols (e.g. SPDY) not always easily usable by clients, or at best use websockets, where HTTP/2 would be preferred to either.
Going more DIY and using JSON over raw byte streams over sockets is a middle ground- it provides the necessary request/response semantics in a simple library (via the net pacakge) and supports the transports we want (TCP, UNIX sockets), but still introduces unnecessary burdens on clients:
Dealing with raw byte streams isn't usually a great client developer experience (go being an exception)
Decoding JSON from remote streams doesn't leave much room for error handling outside of writing our own protocol (see conclusion 1 above). Clients would have to interpret response wrappers expressed as JSON, and the server would have to reliably ship JSON always- without a fully formed JSON response, there's no header or protocol level response slot to place any information.

We've decided to just stick with HTTP. For now, we think it provides the best client experience:

Every stack under the sun knows how to make requests and decode the response
It provides a natural vector for error management and metadata for free at the protocol level
It supports both the request/response semantics we want and also facilitates a flawed but usable means for an event stream
We can talk HTTP over TCP and UNIX sockets with no additional effort
When HTTP/2 support becomes widespread, the event streaming support improves with minimal effort

The HTTP interface would become the usual sort of JSON-based REST API. For example:

GET /threads would return []api.Thread
POST /breakpoints using api.Breakpoint creates a breakpoint and returns []api.Breakpoint
POST /command using api.SwitchThread switches the thread and returns the newly selected api.Thread
GET /watch/threads would block until the server thread list changes, at which time it would return the new []api.Thread

The client would then be required to re-establish a new watch to receive further updates (this is one of the usability drawbacks of HTTP; ideas welcome)

The API itself is still a work in progress as we've churned through experiments for this feature. Suggestions are very welcome. The API should be easy to revise as we go.
@dlsniper - do you feel like such an HTTP interface would be usable for the IDEA integration?
I'm going to begin prototyping again with this new model while awaiting additional comment. Please let me know if I've made mistakes in my representation of any of the choices or have missed some option that would warrant further exploration. A lot of this stuff I've been learning on the fly; I don't consider myself an expert on any of these subjects.

@derekparker
One implementation detail followup: As I started down the refactor path again this morning, I realized that writing all the HTTP request/response handling stuff was going to get tedious and messy fast. With Kubernetes/OpenShift, we've had great success building an extremely robust HTTP API with go-restful. If you'd be okay with bringing in the external dependency, we could move much, much more quickly with the server and focus on features and docs rather than plumbing.
If you're still wary about bringing in another dependency, I'm happy to just put in the work to write custom handling. Just wanted to bring it up since I'm convinced it would be a huge time saver and since I consider the library vetted at this point.

@ironcladlou I'm fine with bringing that in.

+1 to go-restful
On Thu, Mar 26, 2015 at 9:40 AM, Derek Parker<EMAIL_ADDRESS>wrote:

@ironcladlou https://github.com/ironcladlou I'm fine with bringing that
in.
—
Reply to this email directly or view it on GitHub
https://github.com/derekparker/delve/pull/88#issuecomment-86517035.

I've rebased and squashed the current progress. The latest commit cleans up all my work, which should reflect all feedback to date. Please take a look at the general architecture and API and let me know what you think. Other than watches, I think I have a decent slice of functionality covered in a way which should serve as an example of how the rest of it will look if I keep going down this road.

@ironcladlou just looked this over and it's really coming together. Looks great so far.

@derekparker @pmorie
The last big API piece to flesh out is all the stuff you can get from the info terminal command. Sources, functions, package vars, locals, all that stuff. I need to spend a little more time really thinking about it. Any thoughts on a possible consolidated API structure to hold those things, or should each piece of information be split out somehow into its own struct? How much would we want to get from a single call for these items?

@ironcladlou A consolidated representation would be nice, but short of that we can get pretty far with serializing []*api.Variable for info {locals, vars, etc..}. And info {sources, functions} can really just be []string serialized.

Agree w/ @derekparker
On Mon, Mar 30, 2015 at 10:00 PM Derek Parker<EMAIL_ADDRESS>wrote:

@ironcladlou https://github.com/ironcladlou A consolidated
representation would be nice, but short of that we can get pretty far with
serializing []*api.Variable for info {locals, vars, etc..}. And info
{sources, functions} can really just be []string serialized.
—
Reply to this email directly or view it on GitHub
https://github.com/derekparker/delve/pull/88#issuecomment-87897948.

Commit 67ff1e9d19e70e2b70ca4f5dede81d52a298f161 refactors the Thread API- check it out.
For sources, since they're process scoped, I was thinking make that a separate call (it could be a lot of data and should support filtering). For package vars, it's a blend of process scoped (all functions in the symbol table) bound with thread scoped values. We're going to probably want a separate filterable call for that as well since it could be a large amount of data.

For sources, since they're process scoped, I was thinking make that a separate call (it could be a lot of data and should support filtering).

It should definitely support filtering, as it currently does (regexp filter).

For package vars, it's a blend of process scoped (all functions in the symbol table) bound with thread scoped values. We're going to probably want a separate filterable call for that as well since it could be a large amount of data.

Yeah, that makes sense to me.

@ironcladlou looks like this is getting there. I'll have time to do a more in-depth review either tonight or tomorrow.

@ironcladlou looks like this is getting there. I'll have time to do a more in-depth review either tonight or tomorrow.

Great, thanks!

Pretty sure all that's left is goroutines which I'm skipping for now as it will require refactoring to variables.go to accept a thread ID and also to return a data structure (or at least print to an injected io.Writer etc.)
In general I think a lot of the functions in DebuggedProcess which assume CurrentThread could instead take a thread ID which would make integrating easier... the notion of the "current thread" is starting to seem more to me like a client concept since on the requesting side you can really get most of these bits of info for any thread you have the ID of, not just the main or "currently selected" thread. Hope that makes sense. I don't think we should do that now, just wanted to mention it as a future consideration. I already represent package variables and eval symbol this way in the outward API: GET /threads/{id}/eval/{symbol} and GET threads/{id}/vars, which the terminal fills in with the "current" thread ID.
Still lots of polish and testing and all that to do, but I think the broad strokes are in. I haven't done an example of a watch/notification yet since I just didn't run into the need, so I'll hold off on that until after some more code review has occured.

Also noting here so I don't forget: still need to implement attach to an existing process. Holding off anything else until I get some more review, though.

In general I think a lot of the functions in DebuggedProcess which assume CurrentThread could instead take a thread ID which would make integrating easier... the notion of the "current thread" is starting to seem more to me like a client concept since on the requesting side you can really get most of these bits of info for any thread you have the ID of, not just the main or "currently selected" thread. Hope that makes sense. I don't think we should do that now, just wanted to mention it as a future consideration. I already represent package variables and eval symbol this way in the outward API: GET /threads/{id}/eval/{symbol} and GET threads/{id}/vars, which the terminal fills in with the "current" thread ID.

Hmm.. not sure about this. From the clients perspective, very rarely will they deal with a thread directly. They will be dealing with the concept of the process as a whole, and current thread will always be representative of the code that is displayed once the process stops, so when a client wants to evaluate a variable, it is doing so in the context of the "current thread".
The only time I can imagine a client needing to deal with a thread directly is when they switch to it. Do that make sense?
I've thought it would be cool to display multiple threads at the same time within a terminal, but I digress.
I'll be looking this over more thoroughly this evening and I'll be able to give more feedback then so you can keep moving forward with this.

@derekparker @ironcladlou

The only time I can imagine a client needing to deal with a thread
directly is when they switch to it. Do that make sense?

I had a draft typed up that failed to send earlier this this same idea
typed up, +1
Also agree it would be interesting to see multiple threads at the same time
On Wed, Apr 1, 2015 at 6:06 PM, Derek Parker<EMAIL_ADDRESS>wrote:

In general I think a lot of the functions in DebuggedProcess which assume
CurrentThread could instead take a thread ID which would make integrating
easier... the notion of the "current thread" is starting to seem more to me
like a client concept since on the requesting side you can really get most
of these bits of info for any thread you have the ID of, not just the main
or "currently selected" thread. Hope that makes sense. I don't think we
should do that now, just wanted to mention it as a future consideration. I
already represent package variables and eval symbol this way in the outward
API: GET /threads/{id}/eval/{symbol} and GET threads/{id}/vars, which the
terminal fills in with the "current" thread ID.
Hmm.. not sure about this. From the clients perspective, very rarely will
they deal with a thread directly. They will be dealing with the concept of
the process as a whole, and current thread will always be representative of
the code that is displayed once the process stops, so when a client wants
to evaluate a variable, it is doing so in the context of the "current
thread".
The only time I can imagine a client needing to deal with a thread
directly is when they switch to it. Do that make sense?
I've thought it would be cool to display multiple threads at the same time
within a terminal, but I digress.
I'll be looking this over more thoroughly this evening and I'll be able to
give more feedback then so you can keep moving forward with this.
—
Reply to this email directly or view it on GitHub
https://github.com/derekparker/delve/pull/88#issuecomment-88646906.

The API I provided allows for both, and we could also provide a simplified client method which effectively gets the current one. This requires the client to get the current thread and then get the info for that thread (what terminal currently does manually.)
If you both think it should usually be contextual based on the selected thread, I can change the API and now the client needs to make just one call, but if you ever want to display multiple thread info, you'd have to switch/get repeatedly.
If the former is the most common use case, maybe the API should be simpler to facilitate it.

On Apr 1, 2015, at 6:10 PM, Paul Morie<EMAIL_ADDRESS>wrote:
@derekparker @ironcladlou

The only time I can imagine a client needing to deal with a thread
directly is when they switch to it. Do that make sense?

I had a draft typed up that failed to send earlier this this same idea
typed up, +1
Also agree it would be interesting to see multiple threads at the same time
On Wed, Apr 1, 2015 at 6:06 PM, Derek Parker<EMAIL_ADDRESS>wrote:

In general I think a lot of the functions in DebuggedProcess which assume
CurrentThread could instead take a thread ID which would make integrating
easier... the notion of the "current thread" is starting to seem more to me
like a client concept since on the requesting side you can really get most
of these bits of info for any thread you have the ID of, not just the main
or "currently selected" thread. Hope that makes sense. I don't think we
should do that now, just wanted to mention it as a future consideration. I
already represent package variables and eval symbol this way in the outward
API: GET /threads/{id}/eval/{symbol} and GET threads/{id}/vars, which the
terminal fills in with the "current" thread ID.
Hmm.. not sure about this. From the clients perspective, very rarely will
they deal with a thread directly. They will be dealing with the concept of
the process as a whole, and current thread will always be representative of
the code that is displayed once the process stops, so when a client wants
to evaluate a variable, it is doing so in the context of the "current
thread".
The only time I can imagine a client needing to deal with a thread
directly is when they switch to it. Do that make sense?
I've thought it would be cool to display multiple threads at the same time
within a terminal, but I digress.
I'll be looking this over more thoroughly this evening and I'll be able to
give more feedback then so you can keep moving forward with this.
—
Reply to this email directly or view it on GitHub
https://github.com/derekparker/delve/pull/88#issuecomment-88646906.

—
Reply to this email directly or view it on GitHub.

The API I provided allows for both, and we could also provide a simplified client method which effectively gets the current one. This requires the client to get the current thread and then get the info for that thread (what terminal currently does manually.)

Definitely it is planned to be able to execute arbitrary commands against any given thread. If you want to build things out to be able to accept arbitrary thread ids that works for me, however the default in that case should always be CurrentThread.

Knocked out the more minor items and started discussion on the bigger ones.

@derekparker @pmorie

Definitely it is planned to be able to execute arbitrary commands against any given thread. If you want to build things out to be able to accept arbitrary thread ids that works for me, however the default in that case should always be CurrentThread.

I resoved this by adding support for everything:

GET /threads/{id}/eval/{symbol}
GET /eval/{symbol (current thread)
GET /threads/{id}/vars
GET /vars (current thread)

The Go client supports both, and the terminal uses the "current thread" variants.

+1
On Wed, Apr 1, 2015 at 10:01 PM Dan Mace<EMAIL_ADDRESS>wrote:

@derekparker https://github.com/derekparker @pmorie
https://github.com/pmorie
Definitely it is planned to be able to execute arbitrary commands against
any given thread. If you want to build things out to be able to accept
arbitrary thread ids that works for me, however the default in that case
should always be CurrentThread.
I resoved this by adding support for everything:

GET /threads/{id}/eval/{symbol}

GET /eval/{symbol (current thread)

GET /threads/{id}/vars

GET /vars (current thread)

The Go client supports both, and the terminal uses the "current thread"
variants.
—
Reply to this email directly or view it on GitHub
https://github.com/derekparker/delve/pull/88#issuecomment-88691905.

@ironcladlou that works.

Refactored the server/debugger setup/shutdown mechanics. Added (untested) attach support. To stop the debugger, you issue a GET /detach?kill=true. I'm not crazy about using a GET but it gets the discussion started. I wasn't sure about adding arguments to the command path.
The client call to Detach is synchronous, so when the terminal gets a result Detach, it knows the debugger is done and the process detached and/or killed, and the terminal can then just exit and no futher action is necessary.
I had to make a lot of changes to debugger/server for this, so it'll be worth another look.

This is getting really close, and I really like how it's turned out.
Have you given thought to an approach for testing the client/server? I had a thought that refactoring the proctl tests into using the new client/server architecture instead of dealing with proctl directly could be useful, but I haven't settled on how I feel about that.

Addressed most feedback and also rebased to keep up with the new proctl changes.

Have you given thought to an approach for testing the client/server? I had a thought that refactoring the proctl tests into using the new client/server architecture instead of dealing with proctl directly could be useful, but I haven't settled on how I feel about that.

I haven't thought about tests too much yet given the amount of churn. For the client, tests would be pretty trivial; the only expectations are really around like, does a call form the right URL, testing the generic request handling parts, etc.
For the server, again, kind of weak tests: do we generate the right HTTP codes for various inputs and outputs from Debugger calls, etc.
For Debugger itself, things get a little more weird. DebuggedProcess isn't abstracted by any interface, so the only real tests we could do would be integrating with a real process, I think (which seems to be what proctl_test does).
If DebuggedProcess (and some of its outputs) were an interface, I could probably make decent tests for Debugger. One thing that's been nagging at me is the similarity between DebuggedProcess and Debugger conceptually... Debugger is like a thin service layer wrapping access to DebuggedProcess in a thread-safe way, and ultimately is either re-exposing methods (e.g. Sources()) or providing aggregate wrappers (e.g. BreakPoints()) and with serializable types for input/output.
I don't have any answers yet, just these random thoughts floating around. What interfaces would be helpful, is there any way to consolidate anything, etc.

Speaking of testing the HTTP server, I'd also want an interface to Debugger itself, which nags at me even more (a Debugger interface and impl and a DebuggedProcess interface and impl, which are going to look really, really similar except for their input/output types).

Something tells me there's a way to collapse Debugger and DebuggedProcess somehow, but it's not clear to me yet. I don't know whether it's important right now, either.

@ironcladlou, it's nothing that can't be done in the future
On Fri, Apr 3, 2015 at 3:07 PM, Dan Mace<EMAIL_ADDRESS>wrote:

Something tells me there's a way to collapse Debugger and DebuggedProcess
somehow, but it's not clear to me yet. I don't know whether it's important
right now, either.
—
Reply to this email directly or view it on GitHub
https://github.com/derekparker/delve/pull/88#issuecomment-89393102.

Three outstanding things that affect the API:

Breakpoint addition refinements (see this thread)
Source context (command.go#printcontext); thinking about adding the context string array to the DebuggerState returned from step, continue, etc. Is this something the server should be responsible for delivering?
Goroutine info (needs some proctl mods to allow injectable io.Writer or something, didn't want to step on anything @derekparker had going on in that package)

Two more outstanding things:

When returning DebuggerState, populate the current breakpoint (if any). I need some advice from @derekparker on how to do this. DebuggedProcess.resume knows how to detect whether the current PC represents a breakpoint, but then it steps around.
Right now, the continue command will block until the program ends or a breakpoint is hit (or until the next line is hit, depending on whether we're stepping, etc.) This seems correct, but are there any concerns around HTTP timeouts between the client and server when letting the program just run for a while?

Source context (command.go#printcontext); thinking about adding the context string array to the DebuggerState returned from step, continue, etc. Is this something the server should be responsible for delivering?

I think this should be a client concern. For IDEs/editors/etc.. they will never need that, because likely they are looking at the source directly.

When returning DebuggerState, populate the current breakpoint (if any). I need some advice from @derekparker on how to do this. DebuggedProcess.resume knows how to detect whether the current PC represents a breakpoint, but then it steps around.

trapWait in proctl/proctl.go should be responsible for setting the current breakpoint on the DebuggedProcess or wherever would be most appropriate, since it knows when a breakpoint has been hit and has been modified recently to find it as well.

@derekparker

Source context (command.go#printcontext); thinking about adding the context string array to the DebuggerState returned from step, continue, etc. Is this something the server should be responsible for delivering?

I think this should be a client concern. For IDEs/editors/etc.. they will never need that, because likely they are looking at the source directly.

Is the absolute file path stored in the DWARF information? The way this is being used right now in the CLI is interesting. The assumption is that the user will have the source in a locally accessibly absolute path encoded in the debugging info. I imagine that IDEs will typically extract part of the path to match up with sources defined in the project's scope so that users can debug stuff without having to place sources in special arbitrary locations dictated by the binary.
I'll take a shot at making Terminal behave like it does today: assuming the paths encoded in the binary are available locally as-is.

When returning DebuggerState, populate the current breakpoint (if any). I need some advice from @derekparker on how to do this. DebuggedProcess.resume knows how to detect whether the current PC represents a breakpoint, but then it steps around.

trapWait in proctl/proctl.go should be responsible for setting the current breakpoint on the DebuggedProcess or wherever would be most appropriate, since it knows when a breakpoint has been hit and has been modified recently to find it as well.

You want to add that feature to master and then I can use it? I'm reluctant to widen the scope of this PR to include core features in the proctl package.

You want to add that feature to master and then I can use it? I'm reluctant to widen the scope of this PR to include core features in the proctl package.

Yeah, I can put that into master.

printcontext is now fixed.

@derekparker Not sure yet what happened here, so I'm hoping something will stand out at you. Right now I assume I somehow messed up in my branch.
Take a look at this gist and see how I got continue to hang after this sequence:
step->add break->continue->step->locals query->step->next->continue
I then got step->next to hang. I added that scenario to a second file in the gist.
Each scenario includes a Go thread dump of the hung delve process.
The test is using `_fixtures/integrationprog.go. Info about my OS/runtime is also available.

Hmmm.. I'll take a closer look this afternoon and see if anything jumps out. A quick glance through did reveal something to me, however it's a known issue I'm working on fixing right now with the next implementation, some of the internal proctl stuff is going through some incremental improvements at the moment.

Some of the other hangs I have been seeing were in fact channel deadlocks due to poor error handling in my code (for example, if the process fails to launch, commands would block due to the debugger function processing loop not running). I made the debugger have a stateful running bool which is used to guard against that type of thing, and now it's much more stable when error conditions arise.

@derekparker Was implementing halt and happen to be doing native development on OSX, and what I'm seeing is that this call to C.mach_port_wait (invoked through DebuggedProcess.Continue) is continuing to block even after a concurrent call to DebuggedProcess.Halt succeeds. Haven't tested it in linux yet. Any clues?

@ironcladlou yeah, I know the issue there. On OSX Halt should be created a mach exception. It shouldn't be a difficult fix, I will have some time tomorrow morning and I can get that taken care of.

@ironcladlou will be taking a look today and outline what's left before merging. Should also be able to fix the above issue as well.

@derekparker

will be taking a look today and outline what's left before merging. Should also be able to fix the above issue as well.

Sounds good. I went ahead and pushed the halt support. I was able to get the same erroneous halt behavior in linux inside a docker container, but haven't had a chance yet to diagnose or get a stack dump, so you might want to double check the linux impl while you're at it.

@ironcladlou yeah, I'm taking a look into both now, have some changes in mind.
It looks like you've gotten all of the current functionality covered in this refactor.  I feel like this PR could be merged in this week, barring any huge issues.
I've been thinking about testing, and haven't landed on solid ground there. My current thought process is that all tests in proctl_test.go should be converted into using a client, and not DebuggedProcess directly. To me it makes sense because that file is integration style tests anyways, and that would nicely cover testing this refactor.
Thoughts?

@derekparker

@ironcladlou yeah, I'm taking a look into both now, have some changes in mind.

It looks like you've gotten all of the current functionality covered in this refactor. I feel like this PR could be merged in this week, barring any huge issues.

I've been thinking about testing, and haven't landed on solid ground there. My current thought process is that all tests in proctl_test.go should be converted into using a client, and not DebuggedProcess directly. To me it makes sense because that file is integration style tests anyways, and that would nicely cover testing this refactor.

Thoughts?

Hmm, I really like this idea. Functionally it would continue to cover what it does today, but also cover the client/server implicitly, and as a bonus opens the door for adding cases more specific to the client/server interactions. And I think it'll make the test code a bit more compact in some cases.
Although, looking over proctl_test.go I noticed at least two themes in my brief scan:

high-level functional tests, shallow state analysis: e.g. if I step, do I end up at the expected spot in the source
lower-level correctness tests, deep state analysis: e.g. return address verification, and some others doing lots of assertions on registers and addresses.

We didn't include some of the lower level pieces of state in the API (e.g. registers, dwarf info, symbol tables), so tests which use the client are limited to state we're exposing publicly. Functionally these would be good tests, but their assertions would be weaker with today's API.
Do you see us just exposing more and more stuff proctl state to maintain the full assertion capabilities but in a full integration test context, or drawing some line between client based and proctl based integration tests, or something else? Hope that makes sense.

Do you see us just exposing more and more stuff proctl state to maintain the full assertion capabilities but in a full integration test context, or drawing some line between client based and proctl based integration tests, or something else? Hope that makes sense.

That definitely makes sense. I think where possible let's use the client/server, but drop down into direct access to DebuggedProcess when necessary. Maybe even split them into two files, i.e. proctl_test.go and proctl_integration_test.go, or something like that.

Stadium full of Gophers: "MERGE! MERGE! MERGE! MERGE!" and let @dlsniper do his magic on the IDEA plugin...

Stadium full of Gophers: "MERGE! MERGE! MERGE! MERGE!" and let @dlsniper do his magic on the IDEA plugin...

LOL. Speaking of which, I pinged but haven't heard from @dlsniper on this PR yet. I was really hoping to get another IDE integrator's thoughts on the API/implementation. Feel free to jump in!

@derekparker
I ported a couple of integration tests in https://github.com/ironcladlou/delve/commit/cd0c0788a2d61cfdd6c55c66970dca90e07ba9a5. I wanted to get your take on it before I went much further.
(Random aside, as noted in the commit: why oh why did I use glog...)

Glog: closed for business, don't even ask, thanks!
On Mon, Apr 13, 2015 at 9:34 PM Dan Mace<EMAIL_ADDRESS>wrote:

@derekparker https://github.com/derekparker
I ported a couple of integration tests in ironcladlou@cd0c078
https://github.com/ironcladlou/delve/commit/cd0c0788a2d61cfdd6c55c66970dca90e07ba9a5.
I wanted to get your take on it before I went much further.
(Random aside, as noted in the commit: why oh why did I use glog...)
—
Reply to this email directly or view it on GitHub
https://github.com/derekparker/delve/pull/88#issuecomment-92554908.

@ironcladlou those test examples look great, I think that's the way to go. Also, I'm all for stdlib logging, as you mentioned in your code comment, we really don't need anything fancy wrt logging.

@derekparker re: the registers issue with TestNext, I figured something out. Here's how next is invoked:
https://github.com/ironcladlou/delve/blob/http-api/service/debugger/debugger.go#L299
After that completes, we gather up the new state to return:
https://github.com/ironcladlou/delve/blob/http-api/service/debugger/debugger.go#L320
During the state gathering, we extract locals using another withProcess call:
https://github.com/ironcladlou/delve/blob/http-api/service/debugger/debugger.go#L452
It's entirely unclear to me how, but the call to ThreadContext.LocalVariables after the Next() call somehow screws up the FrameDescriptionEntry.CIE.InitialInstructions array for subsequent calls to executeDwarfProgramUntilPC via EstablishFrame. In the case we are seeing, the offset instruction is getting stripped out, and the whole set of instructions seems unrelated to the Next call at hand.
Removing the call to LocalVariables keeps the instruction set from getting jumbled and everything works fine. I'm not sure if I've managed to accidentally introduce some concurrent proctl calls or leak something across thread boundaries yet, but at least I made some progress.

@derekparker Based on your IRC comment, I changed the halt api command to use DB.RequestManualStop() rather than DB.Halt(), fixing my issues. Can you explain the intended use of DB.Halt() so I don't screw up again? :grin:

Maybe answered my own question... Halt() signals thread groups, but RequestManualShutdown actually signals the process, which will also halt the child thread groups and also prevent new threads from spawning, right?

I now realize I lost the SIGINT trap which should be calling RequestManualShutdown. Will fix.

Maybe answered my own question... Halt() signals thread groups, but RequestManualShutdown actually signals the process, which will also halt the child thread groups and also prevent new threads from spawning, right?

RequestManualStop sets the halt flag on DebuggedProcess which is used in trapWait to let it know that it's ok that it stopped for an otherwise unknown reason. Also, on Linux it signals the process so that the trapWait call that's already blocking on another threads will return. On OSX it raises a mach exception so that the call to mach_msg returns.
DebuggedProcess.Halt iterates through each thread and calls Thread.Halt, which, depending on OS, either sends a signal and waits for it (Linux), or calls thread_suspend (OSX). By the time DebuggedProcess.Halt returns all threads should be stopped.

@ironcladlou this looks great. After the changes mentioned above are fixed I plan on getting this merged, hopefully by tomorrow at the latest.
Thanks again for all your work on this. I know this ended up being a significant amount of work to get this refactor in, and I really appreciate how available/responsive you've been throughout the lifecycle of this PR.

Thanks again for all your work on this. I know this ended up being a significant amount of work to get this refactor in, and I really appreciate how available/responsive you've been throughout the lifecycle of this PR.

My pleasure, this has been fun. I've got a followup branch going with a variety of little things which I've found necessary for Sublime integration, so I hope to keep hacking away.

@ironcladlou I keep getting a failure on TestClientServer_exit on OSX. Also, could you update the Makefile for the updated directory structure and to manually run the integration tests.

@ironcladlou also, a heads up: just pushed to master, should not affect you, but you should rebase onto it.

Good thing you tried this- I haven't been testing on OSX lately. Might not get to this till tomorrow, but will sort it out ASAP.

On Apr 19, 2015, at 5:14 PM, Derek Parker<EMAIL_ADDRESS>wrote:
@ironcladlou I keep getting a failure on TestClientServer_exit on OSX. Also, could you update the Makefile for the updated directory structure and to manually run the integration tests.
—
Reply to this email directly or view it on GitHub.

Rebased.

@derekparker Some data related to the wedging I'm experiencing on darwin...
Immediately before the trapwait call in the proctl next function, the correct temporary breakpoints were set:
bp: &proctl.BreakPoint{FunctionName:"main.testnext", File:"/Users/dan/Projects/go/src/github.com/derekparker/delve/_fixtures/testnextprog.go", Line:19, Addr:0
x2192, OriginalData:[]uint8{0x48}, ID:3, Temp:true, hardware:false, reg:0}
bp: &proctl.BreakPoint{FunctionName:"main.main", File:"/Users/dan/Projects/go/src/github.com/derekparker/delve/_fixtures/testnextprog.go", Line:40, Addr:0x23e
4, OriginalData:[]uint8{0x48}, ID:4, Temp:true, hardware:false, reg:0}

The call to continue the thread returns, but the trapWait call never unblocks. That's all I have for now. Seems to be working fine in linux.

@derekparker
Some more useful debugging information:
So, leading up to the execution of the very first TextNextGeneral scenario, both the int and unit tests have identical proctl call stacks in terms of calls to Next, Continue, etc. Then, during the first scenario for the test, in the call to Next, I'm observing two different results for the same call to PtraceCont:

In the unit test, ultimately ThreadContext.resume is called to perform the next. This calls sys.Syscall6 with PTRACE_CONT and returns 0x10, which causes the next logic to fall through to C.resume_thread; this is how all the Next calls leading up to the blockage behave.
In the integration test, the exact same sys.Syscall6 call at the same relative point in the call flow returns 0x00 even though up to that point it had been returning 0x10 like usual. The 0x00 response means C.resume_thread is never called, and so the Wait4 never unblocks.

So something in the client/server branch is causing the ptrace_darwin.go sys.Syscall6(PTRACE_CONT) call to start erroneously returning 0x00.

@ironcladlou
I am working on golang based cluster tools for machine learning and other compute heavy use cases. These are heavy on computation with many servers involved and also embedded and mobile devices. I need to spread the computation because the embedded and mobile devices act as sensors for the machine learning system, and golang can run on all these devices so its very useful.
What would be awesome is the ability to debug and trace across many devices - i got used to this with Erlang :)  From what i see of the discussion and code, the debugger will be accessible via different protocols. In my case i am using http2, which is a multiplexing protocol. I am also using MQTT and working on QUIC integration.  So its great that you and @derekparker  maintained a protocol agnostic layer in this architecture !
I would be happy to help work on this with you if it interests you. i am also interested in using open shift for the machine learning cluster, and so any integration we can do would be great .

@derekparker
Check out these tidbits from dtruss leading up to the wedging.
Here is the proctl unit test leading up to the blocking wait call. Notice the typical ptrace calls that return 0x10, and the simple select which catches the state change from the continues.
Now, look here at the integration test for the same code path. Notice how we're getting two sigreturns here and here? And how suddenly after those mysterious signals, ptrace is returning 0x0 and the selects block forever.
So, what is this:
sigreturn(0xC208009F48, 0x1E, 0x1C)		 = 0 Err#-2

According to signal(3) 0x1E could be:
30    SIGUSR1      terminate process    User defined signal 1

I feel like the root problem is still just beyond my comprehension, but I'm hoping this extra detail will ring some bells in that bulging UNIXy brain of yours.

@joeblew99

I would be happy to help work on this with you if it interests you.

The more the merrier. This PR was very close to being merged until we found a last minute bug that only manifests on OSX. There will be followups to support other transports, headless mode, etc.

i am also interested in using open shift for the machine learning cluster, and so any integration we can do would be great .

I work on the OpenShift team, and have plans to demonstrate integration with Delve (although there are some prerequisite things that need done in Docker/libcontainer to support it, like shared process namespaces). I'm danmace on FreeNode throughout the working week; feel free to drop by and chat.

@derekparker
I got lucky and found another clue which I was able to use to similarly break the tests in master. No fix yet, but this has to be useful information. Will chat more on IRC. All I'll say for now is tmux makes it really easy to break the test. :grin:

@ironcladlou regarding out conversation on IRC about the signals issue:
Remember when I said I had something that worked "like a charm"? I lied, I got too excited, hah.
In order for Delve to leverage PT_SIGEXC on Darwin, the following things must happen, in order:

Fork the child process
(In parent) acquire mach task, and set up ports (including exception port)
(In child) the follow ptrace calls must be made: PT_TRACEME and then PT_SIGEXC
Exec the process we want to debug

I looked into modifying the stdlib to see if I could get a patch in to optionally call a function after fork but before exec, but in the stdlib implementation after fork you are not allowed to malloc or modify the stack, which means no non-asm function calls. Which means that for Darwin Delve will need its own fork/exec implementation. Not sure how that'll play with cgo, but I'm going to work on something tonight and try and get it into master.

@ironcladlou try rebasing on top of master and confirming the issue is resolved.

@derekparker I think we're in the clear. Awesome work. :metal::feelsgood::metal:

@derekparker TestHalt was failing on linux due to a race I fixed in #108. Take a look.

@ironcladlou Merged!
I think there is still some refactoring to do, especially around everything that's shared between proctl/integration tests, and I'd like to pull over to precompiling fixtures setup function into proctl- but I feel it was at a point where it could be merged into master and then worked from there.

Epic - thanks @ironcladlou for all the hard work here.

@ironcladlou @pmorie @pnasrat Many thanks for the all the work done here.

WOOHOO! Awesome work guys! Now we just need GUI integration with IDEA...

:tada: Yay!
Will begin followups soon for the stuff you mentioned and also for the server/daemon mode.

Thank you guys for your work. I haven't had time to do anything else due to personal reasons but I've reviewed the API and it looks good for IDEA. I'll start poking around with that soon. but once again, @ironcladlou and @derekparker thank you!


Add button for user to copy achievement uuid

Description

Added a button to achievement card in the achievement control that gives the user access to the uuid of the achievement, as requested by the game team.
Type of change

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] This change requires a documentation update
[ ] Code quality improvements

How to test

Checklist

[x] I have tested this code
[ ] I have updated the documentation

Pull Request Test Coverage Report for Build<PHONE_NUMBER>

0 of 4 (0.0%)  changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.008%) to 30.799%

Changes Missing Coverage
Covered Lines
Changed/Added Lines
%

src/commons/achievement/control/achievementEditor/AchievementUuidCopier.tsx
0
4
0.0%

Totals

Change from base Build<PHONE_NUMBER>:
-0.008%

Covered Lines:
3449

Relevant Lines:
10420

💛  - Coveralls


Add start/stop parameters to StartStopSupport

Motivation:
A user sometimes wants to pass an argument when starting or stopping
something.
Modifications:

Added two more type parameters to StartStopSupport.
Added StartStopSupport.start(arg, failIfStarted)
Added StartStopSupport.stop(arg)
Modified the parameters of StartStop.doStart(), doStop() and
listener notification methods.

Result:

A user can pass a startup/shutdown argument when using
StartStopSupport.
StartStopSupport has breaking changes.

Thanks for reviewing. :smile:


Ledger Interval API

Adds an initial implementation of the Plutus.V1.Ledger.Interval in eopsin

Yeah I'm not sure either. The names are pretty generic so I would prefer not importing them by default. we can also simply not import in the prelude by default until we have a fancy import + renaming system

@nielstron This should be ready to merge. Thanks!


Adding support for external C functions that have integer (or empty) args and/or returns

Starts addressing @https://github.com/rust-lang/miri/issues/11
Implementation
Adding support for calling external C functions that have any number of integer arguments (types of integers: i8, i16, i32, i64, u8, u16, u32, u64) and an integer return type (or void).
As suggested in @https://github.com/rust-lang/miri/issues/11, the libffi crate is used to dispatch the calls to external C functions.
Modifications
Main modifications are to:

helper.rs : adding a function call_and_add_external_c_fct_to_context to read the code pointer to the external C function, dispatch the call, and save the return in MIRI internal memory. Handles all conversions between MIRI and C values.
foreign_items.rs : handles the calling of call_and_add_external_c_fct_to_context in helper.rs when a foreign item is encountered. Also adds some structs to model C representations of arguments, and the signature of the external C call.

Testing
Adds tests for the following external functions which are now supported:

int tests:

adds 2 to a provided int (no type of int specified, so autocasts)
takes the sum of its 12 arguments (tests stack spill)
adds 3 to a 16 bit int
adds an i16 to an i64
returns -10 as an unsigned int

void tests

void function that prints from C

🤯
did you mean to open this here? This is not the main miri repository

exploding_head
did you mean to open this here? This is not the main miri repository

We opened this over here for early code review - we'll be sharing a design doc for rust-lang#11 on the issue early next week covering the more complex part of this (syncing with pointers) for discussion.
This PR goes through the steps to support moving simple non-pointer types across FFI.

Thanks for the feedback! I've pushed commits addressing the comments. As you state, the main change is factoring out functionality into helper methods, and introducing macros to cut down on the code duplication (hooray for macros!! 🎉 )

Closing in favor of the real PR


[FEATURE] Add support ZTE H3600 V9

I would like the ZTE H3600 V9 router to be supported.

@Harun348  hello you have acces admin to your zte h3600 ?

@fagmixy hi.yes I have access to admin account

``

@fagmixy hi.yes I have access to admin account

You have digi h3600 can you help me to have password fro admin please

Hi
same here please
greetings

Closing as dupe of https://github.com/mkst/zte-config-utility/issues/76


Change maintainer

This pull request updates the README to list @yknl as the maintainer of blockstack.js.
@yknl has proposed a very exciting roadmap for blockstack.js and our other developer tools for 2019. I can't wait to see where things go!

 Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.You have signed the CLA already but the status is still pending? Let us recheck it.

@hstove

Larry where are the tests for this???

lol!!!


Turn our TableGen file into documentation?

I've noticed in recent PRs that I've had to update the docs as I've changed ops for the GraphBLAS dialect. A lot of the time, I'm simply copying and pasting from the TableGen file. I'm wondering if there's some magical way to have our docs use the TableGen file to generate documentation.

https://github.com/metagraph-dev/mlir-graphblas/pull/206 resolves this issue.


Feature request: add aws_vpn_connection

It would be great to be able grab all this config data:
https://www.terraform.io/docs/providers/aws/r/vpn_connection.html
Great product! Thanks

I second this request.  The vgw command is a nice start, but this would be totally amazing.
I also second the "Great product" sentiment;  much appreciated!

I don't suppose you guys found an alternative tool for this did you?


the queue

I dont like the use of the word pointer or (ptr) for time tracking since it could be confused for a c style pointer. Especially since this code will be used by people outside of hms who may not know java well.

@TomKimsey What do you think is a better name? I couldn't think of anything else, which is how I ended up with pointer/ptr

I was using time tracker for the same reason. timeTracker or queueTimeTracker would work

l

This came up in Nordson as well and I forget what Oliver changed it to.
Marker, index, tracker, position, value

l

This came up in Nordson as well and I forget what Oliver changed it to.
Marker, index, tracker, position, value

changing to tracker

Pointer is descriptive but i think some of the people who use this might not know java well and get confused.

changed pointer to tracker

Moving the string file stuff to another file really cleaned this up.


TextInput demo: Disabled input boxes are targetable and editable using tab

While you cannot click on disabled text boxes I noticed they can be tabbed to and then edited.

Good catch, if you want to do a pull request to fix this all you need to edit is these lines
From:
_renderInput({value, required, type, placeHolder, label}) {
   return html`<input type="text" type$="${type}" placeholder$="${placeHolder}" required?="${required}" class$="mdc-text-field__input ${value ? 'mdc-text-field--upgraded' : ''}" id="text-field" value="${value}" aria-label$="${label}">`;
 }

To:
_renderInput({value, required, type, placeHolder, label, disabled}) {
   return html`<input type="text" type$="${type}" placeholder$="${placeHolder}" required?="${required}" disabled?="${disabled}" class$="mdc-text-field__input ${value ? 'mdc-text-field--upgraded' : ''}" id="text-field" value="${value}" aria-label$="${label}">`;
 }

Fixed by #297


Check that transaction fees are reasonable

@raphjaph Take a look! I added a checks that target fee rates are equal to the target fee rate. This requires avoiding calling .build() before fees are added, since that will now panic, so I changed fee estimation to be done on a dummy transaction with null inputs, instead of calling self.build().


mousewheelControl param is not changing when using enableMousewheelControl & disableMousewheelControl methods

This is a (multiple allowed):

[x] bug

[ ] enhancement

[ ] feature-discussion (RFC)

Swiper Version: 3.4.1

Platform/Target and Browser Versions: Latest OSx, Latest Chrome.

What you did
I've initialised slider with mousewheelControl: true param.
I had to disable mousewheel.
slider.disableMousewheelControl()
Expected Behavior
slider.params.mousewheelControl value should be changed into false when using slider.disableMousewheelControl() or to true when using slider.enableMousewheelControl()
Actual Behavior
slider.params.mousewheelControl  is not been changed
P.S. Remember, an issue is not the place to ask questions. You can use Stack Overflow
for that.
Before you open an issue, please check if a similar issue already exists or has been closed before.

Issue is closed because of outdated/irrelevant/not actual/needed more information/inactivity.
If this issue is still actual and reproducible for latest version of Swiper, please create new issue and fill the issue template correctly:

Clearly describe the issue including steps to reproduce when it is a bug.
Make sure you fill in the earliest version that you know has the issue.
Provide live link or JSFiddle/Codepen or website with issue


Add Tester | QA to Contributor's Journey

Done. See https://www.notion.so/cere/Contribute-9bcbdfc546104611ba844113e8ddbf53

Also updated https://www.notion.so/cere/Join-the-Community-37898c33a91d442b88ff675ed86ff427


Rebranding

Rebrand to Orbit Components.

Old packages deprecated:
https://www.npmjs.com/package/@kiwicom/icons
https://www.npmjs.com/package/@kiwicom/balkan-ui

Done with #74


migrate more unit tests to null safety

https://github.com/flutter/flutter/issues/71511
Migrates unit tests that do not have any null  unsafe code in their transitive closure. this allows the tests to correctly run in sound mode.

Both the google3 and customer testing errors I'm getting are stale. Actually the google3 test errors look like they're coming from the wrong PR


Write documents for Flow 2.0.

Pull Request Test Coverage Report for Build 3928

0 of 0   changed or added relevant lines in 0 files are covered.
6 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.003%) to 35.881%

Files with Coverage Reduction
New Missed Lines
%

azkaban-exec-server/src/main/java/azkaban/execapp/ConditionalWorkflowUtils.java
1
92.5%

azkaban-common/src/main/java/azkaban/jobExecutor/AbstractProcessJob.java
2
64.29%

azkaban-exec-server/src/main/java/azkaban/execapp/JobRunner.java
3
77.54%

Totals

Change from base Build 3924:
-0.003%

Covered Lines:
10323

Relevant Lines:
28770

💛  - Coveralls


Error: Could Not Fetch Catalog Entries

Hi all,
getting the error above Error: Could Not Fetch Catalog Entries -> TypeError: Network request failed on my local host session. Every page has a similar issue. Looking for help.

I would recommend checking either the browser console for more information, or the backend logs to see if there was a startup failure on the backend.


Serial changes v3

Breaking changes

Passing the Clock parameter to Serial by reference.
Serial::usart1/2/3 -> Serial::new.
Serial implements Write<WORD> and Read<WORD> for WORD simultaneously as u8 and u16.

Added

Allow access to the Tx and Rx parts of the Serial without the need for splitting.
Allow Serial reconfiguration by references to the Tx and Rx parts.
Allow Serial release after splitting.

Instead of #414 and #409.

bors r+


template uses incorrect source

On Ubuntu 14.04 running Chef 11.12.4 get the following error:
Recipe: ulimit::default
  * template[/etc/pam.d/su] action create
================================================================================
Error executing action `create` on resource 'template[/etc/pam.d/su]'
================================================================================

Chef::Exceptions::FileNotFound
------------------------------
Cookbook 'ulimit' (0.3.3) does not contain a file at any of these locations:
  templates/ubuntu-14.04/service.erb
  templates/ubuntu/service.erb
  templates/default/service.erb

This cookbook _does_ contain: ['ulimit.erb','domain.erb','su.erb']

Resource Declaration:
---------------------
# In /root/.chef/local-mode-cache/cache/cookbooks/ulimit/recipes/default.rb

 22:     template "/etc/pam.d/su" do
 23:       cookbook ulimit['pam_su_template_cookbook']
 24:     end
 25: end

Compiled Resource:
------------------
# Declared in /root/.chef/local-mode-cache/cache/cookbooks/ulimit/recipes/default.rb:22:in `from_file'

template("/etc/pam.d/su") do
  provider Chef::Provider::Template
  action "create"
  retries 0
  retry_delay 2
  guard_interpreter :default
  path "/etc/pam.d/su"
  backup 5
  atomic_update true
  source "service.erb"
  variables {:conf_lines=>{"pam_rootok"=>{"interface"=>"auth", "control_flag"=>"sufficient", "name"=>"pam_rootok.so"}, "pam_env"=>{"interface"=>"session", "control_flag"=>"required", "name"=>"pam_env.so", "args"=>"readenv=1"}, "pam_env_locate"=>{"interface"=>"session", "control_flag"=>"required", "name"=>"pam_env.so", "args"=>"readenv=1 envfile=/etc/default/locale"}, "pam_mail"=>{"interface"=>"session", "control_flag"=>"optional", "name"=>"pam_mail.so", "args"=>"nopen"}, "pam_limits"=>{"interface"=>"session", "control_flag"=>"required", "name"=>"pam_limits.so"}}, :includes=>["common-auth", "common-account", "common-session"]}
  cookbook_name "ulimit"
  recipe_name "default"
  owner "root"
  group "root"
  mode 420
end

Running handlers:
[2015-05-15T01:37:49+00:00] ERROR: Running exception handlers
Running handlers complete

[2015-05-15T01:37:49+00:00] ERROR: Exception handlers complete
[2015-05-15T01:37:49+00:00] FATAL: Stacktrace dumped to /root/.chef/local-mode-cache/cache/chef-stacktrace.out

No attributes are overridden.
If you look at the compiled resource section, you'll notice that the source is service.erb instead of su.erb
Looking at the Chef 11.12.4 source code, the source should be su.erb: https://github.com/chef/chef/blob/11.12.4/lib/chef/resource/template.rb#L39

Closing since this is not an issue with the ulimit cookbook.
Turns out that this cookbook conflicts with the pam cookbook.
https://supermarket.chef.io/cookbooks/pam
They both update /etc/pam.d/su on Debian/Ubuntu platforms.

If anyone else runs into a similar issue, just set the attribute: default['ulimit']['pam_su_template_cookbook'] to pam if you're using the pam cookbook


Leaderboard not in the right order

I've just had a few rounds of 2048 and someone else beat me but they show up as number 2 even though they clearly beat me:

I mean it still says on the bottom that "Your Position: 2" but shouldn't LenaTheEevee show up above me and be labelled 1 and not 2?
Just a small thing, otherwise the plugin appears to be working great and is lots of fun 😄

I'm sorry I never noticed this issue until I got your closing email.
I'll look into it!

Haha, I'm not using the plugin anymore and thus can't even give any feedback if you ever fix it or do anything about it which is why I closed it.
What I'm saying is you're on your own when you'll be fixing it 😅

Fixed in most recent version


under cygwin the plugin use of xjc -d fails

does the xjc sbt plugin work under cygwin?? note that other plugins and sbt itself is fine.--
[info] Compiling 8 XSD file(s) in {file:/C:/Cygwin/home/cbordel/jcvas/MSE/}mse_ijas_diameterMsg2/compile:xjc
[debug] XJC java command line: -classpath
[debug]
[debug] com.sun.tools.xjc.XJCFacade
[debug] -d
[debug] C:\Cygwin\home\cbordel\jcvas\MSE\ijas\adaptors\mse_ijas_diameterMsg2\generated\xml
[debug] -verbose
[debug] C:\Cygwin\home\cbordel\jcvas\MSE\ijas\adaptors\mse_ijas_diameterMsg2\src\main\resources\schemas\avp.xsd
[debug] C:\Cygwin\home\cbordel\jcvas\MSE\ijas\adaptors\mse_ijas_diameterMsg2\src\main\resources\schemas\avp_flag.xsd
[debug] C:\Cygwin\home\cbordel\jcvas\MSE\ijas\adaptors\mse_ijas_diameterMsg2\src\main\resources\schemas\avp_rule.xsd
[debug] C:\Cygwin\home\cbordel\jcvas\MSE\ijas\adaptors\mse_ijas_diameterMsg2\src\main\resources\schemas\command.xsd
[debug] C:\Cygwin\home\cbordel\jcvas\MSE\ijas\adaptors\mse_ijas_diameterMsg2\src\main\resources\schemas\diameter.xsd
[debug] C:\Cygwin\home\cbordel\jcvas\MSE\ijas\adaptors\mse_ijas_diameterMsg2\src\main\resources\schemas\profile.xsd
[debug] C:\Cygwin\home\cbordel\jcvas\MSE\ijas\adaptors\mse_ijas_diameterMsg2\src\main\resources\schemas\proprietary.xsd
[debug] C:\Cygwin\home\cbordel\jcvas\MSE\ijas\adaptors\mse_ijas_diameterMsg2\src\main\resources\schemas\unsigned32.xsd
[info] :: delivering :: mse#mse;ZZ :: ZZ :: release :: Wed Feb 29 21:28:21 EST 2012
[debug]   options = status=release pubdate=Wed Feb 29 21:28:21 EST 2012 validate=true resolveDynamicRevisions=true merge=true resolveId=null pubBranch=null
[debug] post 1.3 ivy file: using exact as default matcher
[info]  delivering ivy file to C:\Cygwin\home\cbordel\jcvas\MSE\target\ivy-ZZ.xml
[debug]   deliver done (12ms)
[debug] local-mse do not support transaction. overwrite transaction not supported yet
[info]  published ivy to C:/Cygwin/home/cbordel/jcvas/MSE/ant-files/publicCache/local/mse/mse/ZZ/ivy-ZZ.xml
[error] Could not create the Java virtual machine.
[error] Unrecognized option: -d

also of interest i noticed command line use of xjc with full paths seems to need single quoted path names
xjc -d 'C:\Cygwin\home\cbordel\jcvas\MSE\ijas\adaptors\mse_ijas_diameterMsg2\generated\xml'
would like to understand how to use xjc sbt under cygwin.
And this could be a more general problem as i've not yet tried other sbt runs that fork a jvm passing full file names.
bord

Rethinking on the above seems not to be -d option related but presumably that the -classpath argument is empty.
So, decided to delve into the sbtxjc plugin source and right now i get as far as the def jars which is in the lazy val options code
Then in here
up.filter( configurationFilter(config.name) && artifactFilter(type = jarTypes) )
returns an UpdateReport for which
flatMap (.modules) flatMap (.artifacts))
is List(Artifact, File)
which would lead to Seq[File].empty
The updateReport's  flatMap (_.modules) does give a bunch of modules. So this part is not empty.
So, it would just seem that the flatMap (_.artifacts) is at least an intermediate cause for the issue.
Has someone used the sbtxjc with external ivy.xml ?


Escaping/quoting of variable args in commands

Proper handling of quoting in bash is always a pain point, it would be nice if the command substitution escaped special characters which is what zx is doing.
I'm not sure what would be the best way to do this while also ensuring that this doesn't hurt the usage where you do want it to not be escaped and I feel this could use more feedback from the community which this issue might help gather.
The current solution is using shlex.quote() but I think this makes it less straightforward.
Example code:

showargv.py

import sys

print(sys.argv)

simplescript.py

#! /usr/bin/env zxpy

var = "test string"
output = ~f"python3.8 showargv.py {var}"

print(output)

This results in output:
['showargv.py', 'test', 'string']

While I think that more often than not you would want:
['showargv.py', 'test string']

yeah this was discussed in a forum before, and yeah I think a simple shlex.quote in my implementation should fix most of the issues with quoting, shell injection etc.
I just haven't had enough time to test out the library, so thanks for creating the issues :smile:
I'll add a few tests in some time and see if quoting causes any other issues. Otherwise, this should be an easy addition.

I've checked your example in release version 1.4.0, and uploaded it.
Feel free to reopen the issue if you have any questions :smile:


pytorch lightning doesn't seem to support wandb logging adequately

How do I log plt -> plotly plots without converting them to images with pytorch lightning?  cf https://docs.wandb.ai/guides/track/log/plots#matplotlib-and-plotly-plots

Fixed in https://github.com/JasonGross/guarantees-based-mechanistic-interpretability/commit/acde9b61dc8b18990d94a936733993f06cdd9622 with help from @euanong


"You cannot rename this element." error when renaming undeclared variable in JS

VSCode Version: Code 1.12.1 (f6868fce3eeb16663840eb82123369dec6077a9b, 2017-05-04T21:26:50.689Z)
OS Version: Windows_NT ia32 6.1.7601
Extensions:

Extension
Author
Version

vsc-ember-frost
ciena-blueplanet
0.0.2

vscode-es-beautifier
dai-shi
0.2.4

vscode-eslint
dbaeumer
1.2.8

Go
lukehoban
0.6.61

PowerShell
ms-vscode
0.12.2

debugger-for-chrome
msjsdiag
3.1.1

vetur
octref
0.6.6

ejs-language-support
QassimFarid
0.0.1

Ruby
rebornix
0.12.1

All extensions disabled

Steps to Reproduce:

Open a JavaScript file
Remove or comment out the declaration of a variable.
(Optional) Add a statement to initialize the variable without declaring it (so it's global).
Attempt to rename that variable by clicking one of its usages and pressing F2.

Expected Behavior:  All usages of the variable are renamed to the new name, or a helpful error message is shown to explain why it can't be done.
Actual Behavior:  Receive the error "You cannot rename this element."
It appears that removing the declaration of a variable prevents it from being renamed via any of its later usages in the file.  This happens even if the variable is assigned a value and becomes a global.  I have two questions about this behavior, which is why I am opening this issue.

Is this expected, reasonable behavior?  The example below is still valid JavaScript and runs with NodeJS.  I don't often use global variables--this is just an example.
If the behavior is expected, then can the feedback for the user be improved?  The current message "You cannot rename this element." isn't helpful.

Here is an example that demonstrates the issue:
// Here I am, cleaning up an old messy JavaScript file that I probably wrote.
// For whatever reason, I comment out / delete the original variable declarations first.
// var oldLongGrossName = process.env.SOME_ARCHAIC_ENV_VAR;

// The old code might be assigning a value to the variable
// without declaring it first (so it's a global).  It doesn't matter for this issue.
oldLongGrossName = 'm_somethingBtnWndow_lol';

// Then I set up new variable(s) somewhere else, maybe using a config file
// or command line arguments.
var betterName = 'something';

// Now I click 'oldLongGrossName' anywhere in the file and hit F2 to try to rename
// all of its occurrences to 'betterName' (or anything, it doesn't matter).
// I get the following error:  "You cannot rename this element."  I am befuddled.
var config = {
  foo: oldLongGrossName
};

// Since I'm reusing this variable in several places, a global rename would be convenient.
console.log('Probably going to log something about ' + oldLongGrossName);

// So convenient...
var fancyDerivedName = oldLongGrossName + '.txt';

Right now the only way I can see to get this to work is to resurrect / create a declaration for the variable, do the rename, and then delete the variable declaration.  A search and replace would work too, I suppose.
Let me know if I can provide more information or be helpful in some way.

This issue was moved to Microsoft/TypeScript#15684


HDDS-5728. ContainerBalancer should use remaining space to calculate utilization.

What changes were proposed in this pull request?
ContainerBalancer gets a list of datanodes sorted according to their utilizations using NodeManager#getMostOrLeastUsedDatanodes.
There are two ways of calculating utilization:

used space / capacity
(capacity - remaining) / capacity

Used space is obtained using the DU or DF command (SpaceUsageSource), while remaining can either be capacity - used or calculated using the java.io.File#getUsableSpace method. This means the two ways of calculating utilization might give different results (when used is not equal to capacity - remaining).
Currently, NodeManager#getMostOrLeastUsed uses method 2 while balancer uses method 1.
Instead, both should consistently use the same approach. We prefer method 2 since the remaining space calculation will also account for space used by processes other than Ozone. See the documentation in VolumeInfo for further details on calculating remaining space.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5728
How was this patch tested?
Existing UT TestContainerBalancer

@JacksonYao287 @lokeshj1703 please take a look.

@lokeshj1703 I've added a unit test.

@JacksonYao287 do you have any other comments?

@lokeshj1703 thanks for the review , seems we can merge this patch now

@siddhantsangwan Thanks for the contribution! @JacksonYao287 Thanks for the review! I have committed the PR to master branch.

Thanks for the reviews!


Compilation error with -std=c99 if assert.h is included before libxsmm.h

This commit is responsible for the following compilation error. https://github.com/hfp/libxsmm/commit/310270acbddeba0bb7f9c8e733ab397ee306269e
$ cat a.c
#include <assert.h>
#include <libxsmm.h>
$ gcc -std=c99 -I$HOME/src/libxsmm/include -c a.c
In file included from /usr/include/sched.h:34,
                 from /usr/include/pthread.h:23,
                 from /home/jed/src/libxsmm/include/libxsmm_sync.h:469,
                 from /home/jed/src/libxsmm/include/libxsmm.h:51,
                 from a.c:2:
/usr/include/time.h:113:5: error: unknown type name ‘locale_t’
  113 |     locale_t __loc) __THROW;
      |     ^~~~~~~~
In file included from /home/jed/src/libxsmm/include/libxsmm_sync.h:469,
                 from /home/jed/src/libxsmm/include/libxsmm.h:51,
                 from a.c:2:
/usr/include/pthread.h:775:9: error: unknown type name ‘clockid_t’; did you mean ‘clock_t’?
  775 |         clockid_t __clockid,
      |         ^~~~~~~~~
      |         clock_t

I'm using gcc-9.2.0 with glibc-2.30. The same issue is present with clang-9.0.1.

For the record, I think this commit should be reverted. Futzing with __USE_GNU is not supported at all. It is defined automatically if one defines the feature test macro _GNU_SOURCE, but feature test macros must be defined before including headers, so there is no supported way for libraries to set them.
       NOTE: In order to be effective, a feature test macro must be defined be‐
       fore  including any header files.  This can be done either in the compi‐
       lation command (cc -DMACRO=value) or by defining the  macro  within  the
       source  code  before  including  any  headers.  The requirement that the
       macro must be defined before including any header  file  exists  because
       header  files may freely include one another.  Thus, for example, in the
       following lines, defining the _GNU_SOURCE macro may have no  effect  be‐
       cause  the  header <abc.h> itself includes <xyz.h> (POSIX explicitly al‐
       lows this):

           #include <abc.h>
           #define _GNU_SOURCE
           #include <xys.h>

       Some feature test macros are useful for creating portable  applications,
       by  preventing nonstandard definitions from being exposed.  Other macros
       can be used to expose nonstandard definitions that are  not  exposed  by
       default.

Thank you for reporting this issue!

[...] with __USE_GNU is not supported [...]

Yes, __USE_GNU is an outcome of features.h eventually controlled by _GNU_SOURCE. Regarding whether this can/should be controlled, I agree as well. The above two dependencies are now more carefully crafted and the include order of header files wrt libxsmm.h is now tested. I am glad you brought this issue to attention!

Thanks. Your updates work for my environment, though there are still places where _GNU_SOURCE is defined in the header (which a user might not include first). This is unsupported and might upset a user wishing to compile with strict non-GNU features. Have you considered removing libxsmm_sync.h from libxsmm.h? It seems like the vast majority of users don't need this feature, and a user who does could include libxsmm_sync.h directly. I believe this would allow you to remove the _GNU_SOURCE definition from libxsmm_macros.h, thereby containing its side-effects in user code.

Yes, it's mostly used internally and could be contained in our src folder rather than include. I have to check if moving the header is (partially) possible (I need to check what's used in Eigen and Tensorflow).
I can remove _GNU_SOURCE, but need to test first. One change was to not rely on the outcome of what _GNU_SOURCE enables. I have to check what's leftover (even in src directory). Though, LIBXSMM can be used in header-only form, which effectively includes all of our source files.

I think we do not want to weed out our interface wrt "unused" functionality at this point. So far, we expose some functionality that is beyond LIBXSMM's main scope but it is widely used internally and potentially useful outside of our implementation realm. Our sample code might be seen as "external" as well. Though, the sync-layer is also documented.

I'm only suggesting the modest API change that libxsmm_sync.h be included by users who need that functionality. In this case, I assume the issue is pthread_yield, which requires _GNU_SOURCE.
SYNOPSIS
       #define _GNU_SOURCE             /* See feature_test_macros(7) */
       #include <pthread.h>

       int pthread_yield(void);

       Compile and link with -pthread.

I think the safe and recommended thing is to instruct users to define _GNU_SOURCE if they will use this functionality.

I was misreading your comment, you suggested to only remove the sync header from the top-level header libxsmm.h. I will just check if this would break afore mentioned software (mostly TF) and the just do it.
All other things will get fallback code, e.g., pthread_yield. In fact, such code is already present but it must be selected depending on __USE_GNU etc.

Excellent, thanks!

Everything should be now in the master branch, i.e., separated sync layer and no "GNU control".


TCP and UDP update_handle() does not prevent overlapping handles

SocketHandles act as primary keys for sockets. TcpSocket.update_handle() and UdpSocket.update_handle() does not prevent changing into an overlapping handle.
Should this be done to prevent multiple users/application from interfering with each other?

I do not see a good way to solve this..