Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track nrepl-messages per connection #1412

Closed
bbatsov opened this issue Nov 11, 2015 · 11 comments
Closed

Track nrepl-messages per connection #1412

bbatsov opened this issue Nov 11, 2015 · 11 comments
Milestone

Comments

@bbatsov
Copy link
Member

bbatsov commented Nov 11, 2015

While discussing with @expez some issues related to #1399 it became apparent that with the dynamic dispatch and dispatch to multiple connections the current *nrepl-messages* buffer is mostly unusable as you can really figure out which message went where.

We need to either have separate messages buffers per each connection or add connection info the the messages we log right now. Probably option 1 would be better. At any rate we're not shipping 0.10 without something like this.

@expez @Malabarba Any comments?

@bbatsov bbatsov added this to the v0.10 milestone Nov 11, 2015
@expez
Copy link
Member

expez commented Nov 11, 2015

The smallest change would be to just add another kw pair: connection "my-connection" to the message. The kw pair would be ignored by the middleware, and only be useful in the message log, but at some point we might benefit from having that information available down there too.

Having separate message buffers makes comparison more cumbersome, which is probably most of what we're going to do when debugging.

@bbatsov
Copy link
Member Author

bbatsov commented Nov 11, 2015

If we add it to the messages all responses should include it as well, otherwise this will become super messy. Message id tracking is per-connection, so you'll still need a way to determine the correspondence between requests and responses on a connection level. I understand your point, but it seems to me this will be more work.

@expez
Copy link
Member

expez commented Nov 11, 2015

Message id tracking is per-connection, so you'll still need a way to determine the correspondence between requests and responses on a connection level.

If you know the message with id n was for connection c then the reply with id n is also for connection c, right?

While this requires some manual work, I kind of think this is 'good enough'. None of us will be using this feature very much so it's unlikely to cause much frustration.

@bbatsov
Copy link
Member Author

bbatsov commented Nov 11, 2015

As someone who gets to receive a lot of output from this buffer to debug it I definitely don't want to do any manual work. I want to see something and immediately understand what's going on.

If you know the message with id n was for connection c then the reply with id n is also for connection c, right?

The problem is that you might very well get in some situations the same ids from different connections. That's unlikely to happen often, but it's not unfeasible. Especially if you often toggle between clj and cljs.

@Malabarba
Copy link
Member

Maybe I missed something but I think we're over thinking this.
Received replies already have a session keyword, don't they? Just create a messages buffer for each session.

@bbatsov
Copy link
Member Author

bbatsov commented Nov 11, 2015

You have two sessions per connection (at least right now). There's also the possibility of session overlap if you're connected to several servers.

@Malabarba
Copy link
Member

You have two sessions per connection (at least right now).

Then, if the second (tooling) session is ever used, it will be logged to its own buffer too. Sounds like a plus. :)

There's also the possibility of session overlap if you're connected to several servers.

How is a session ID calculated? I always thought it was a semi-random hash or something.

@bbatsov
Copy link
Member Author

bbatsov commented Nov 11, 2015

We should check the source. I just know it's guaranteed to be unique per server.

Then, if the second (tooling) session is ever used, it will be logged to its own buffer too. Sounds like a plus. :)

Still, users will have hard time mapping session ids to connection names. I'm pretty sure about this.

@bbatsov
Copy link
Member Author

bbatsov commented Nov 15, 2015

Btw, we should also consider that cider middleware requests & responses currently don't have a session associated with them and that all connections share the same request ID counter.

@bbatsov
Copy link
Member Author

bbatsov commented Nov 25, 2015

I've stuffed sessions to all requests and went with @Malabarba's suggestion for now. Mapping to connections is not hard, but has a few small implications:

  • the connections are known only to cider
  • you have to go through the list of connections to see their sessions
  • you have to extract the connection "designation" and apply it to the messages buffer

The current solution, while not ideal, is simple and gets the job done (more or less).

@Malabarba
Copy link
Member

This is implemented. Though, as mentioned on gitter, perhaps we can reduce the number of buffers created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants