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

Byebug.start_server: yield later, when actual_port is known #277

Merged
merged 1 commit into from
Sep 28, 2016

Conversation

cben
Copy link
Contributor

@cben cben commented Jun 12, 2016

I didn't find docs or tests for start_server taking a block;
not sure how you intended that to be used and whether you'll consider this backward-compatible.

This allows wait_connection=true to be meaningfully used with port=0.
(If it doesn't return until a client connects,
but a client can't know what port it will listen on,
then here is the only chance to tell the client what port to use.)

My (ab)use case is debugging an app with many background workers.
I wanted to use remote byebug, but I'd have to make each worker listen on a different port, and connect to them all. (To make things worse, workers are sometimes created/destroyed.)
Instead I wrote this kludge: https://gist.github.com/cben/4d4851f63ded79b74d1cb162422ff13b
which at the breakpoint starts a server and a new terminal with a client that connects to it.
I wanted to avoid manually specified ports with retries on collisions; port=0 is perfect.
But I also wanted wait_connection, which is hard to fake externally as the mutex is not exposed...
With this patch that code could be reduced to something like:

def byebug_term
  require 'byebug/core'
  unless Byebug.actual_port
    Byebug.wait_connection = true
    Byebug.start_server('localhost', 0) do  # pick an available port
      system("gnome-terminal -x byebug -R localhost:#{Byebug.actual_port} &")
    end
  end
  Byebug.attach
end

This allows wait_connection=true to be meaningfully used with port=0.
@deivid-rodriguez
Copy link
Owner

@cben First of all, I'm sorry for not replying in such a long time.

You really seem to have thought this through, and I really don't use this part of the debugger. I was not sure what to do here since it's unlikely that I'll find time to check this in the near future.

Anyways, I'm going to merge and just revert the simple patch if it causes problems for people. Expect a new release with this tiny improvement in the next days.

Finally, are you able to add some tests or give some love to the remote debugging bit of byebug? You might be the only one with the knowledge to be able to do that :)

Anyways, thanks a lot, hope the patch will still be useful to you.

@cben
Copy link
Contributor Author

cben commented Sep 27, 2016

Great! I don't have much bandwidth, and my "knowledge" is just reading this one file, but I hope to contribute more, specifically I wanted to make pp work remotely (instead of going to app's stdout).

@deivid-rodriguez deivid-rodriguez merged commit c091a02 into deivid-rodriguez:master Sep 28, 2016
@deivid-rodriguez
Copy link
Owner

deivid-rodriguez commented Sep 30, 2016

Just released 9.0.6 with your change, @cben! Hope it's useful to you.

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

Successfully merging this pull request may close these issues.

None yet

2 participants