Love for remote debugging #406

deivid-rodriguez · 2017-12-31T17:30:25Z

Fixes #82.
Fixes #141.
Fixes #239.
Fixes #274.
Fixes #302.

deivid-rodriguez · 2018-01-04T21:56:34Z

Summoning @MSP-Greg in case you're interested. I've been working on remote debugging support. Not ready yet but looking good. However, I'm getting some failures on Windows and I don't have a Windows machine to debug. In case you would like to help out, I suspect it's something around setting binary mode in streams, but not sure. Regards!

MSP-Greg · 2018-01-04T23:27:53Z

@deivid-rodriguez

I'll have a look at it, might not be able to seriously do so until Saturday... Thanks, Greg

deivid-rodriguez · 2018-01-04T23:40:39Z

That's great, thanks so much!

MSP-Greg · 2018-01-05T02:50:23Z

@deivid-rodriguez

Solution found. See commit in my fork, Appveyor results

Probably not what you (or I) expected. Hope this helps, Greg

EDIT:

I didn't remove the Binmode? commit, I doubt it's required, and (as I recall) it may change the line separator. My test was done with it. Also, the two 'forking' tests I may look at using a Thread, so they'll (maybe) work on Windows.
Unrelated - if you have a minute, you might want to have a look at Ruby Issue 14276. I thought about throwing in my thoughts, but yours would be much better...

MSP-Greg · 2018-01-05T05:11:45Z

Added another patch to swap the fork for Thread. See commit in my fork. Appveyor results.

Appveyor runs with dual cores, Travis has four. The sleep(windows? ? 2 : 1) line may need windows increased to 2.5 or 3, not sure. The first build I ran stopped with a very strange log, second build passed...

deivid-rodriguez · 2018-01-05T15:24:04Z

@MSP-Greg Thanks so much! Your fix makes a lot of sense since only byebug's executable sets up the LOAD_PATH but it can't be used directly on Windows!

Also thanks for letting me know about the ruby-core issue, I commented in there! And also thanks for fixing those tests to not use fork.

I'll either give you access to this branch to add your changes, or cherry-pick them myself!

MSP-Greg · 2018-01-05T16:06:45Z

@deivid-rodriguez

Also thanks for letting me know about the ruby-core issue, I commented in there!

Thank you. I keep an eye on it (and the ruby-core Travis & Appveyor), and I knew you could address the issue(s) well.

I'm also sensitive to issues that involve trunk, If someone is concerned about a given repo/gem functioning with trunk, they're welcome to fork and run their own CI, since many 'mature' repos/gems may not have frequent commits...

I'll either give you access to this branch to add your changes, or cherry-pick them myself!

Whatever is easiest.

Finally, I reverted 'Binmode?' with two commits, both passed.

Thanks, Greg

MSP-Greg · 2018-01-05T19:35:20Z

@deivid-rodriguez

Would you prefer that I delete or revert the Binmode? commit (since it is the last one)?

deivid-rodriguez · 2018-01-05T19:38:12Z

Delete 👍

MSP-Greg · 2018-01-05T20:03:32Z

Sometimes Appveyor... Had this issue last night, where I changed sleep 1 to sleep(windows? ? 2 : 1).

Just upped it to 3, no change. I'm going to check/fix it on my fork. I must be missing something....

FYI - from all my testing on Appveyor (100's of trunk builds, rubygems builds, etc), I've seen lots of intermittent issues...

MSP-Greg · 2018-01-06T18:59:52Z

@deivid-rodriguez

I found what the issue was with my fork passing the one test. I dl'd a diff of the fork, and (oddly) one line was different. I even checked my browser history, and timestamps matched, so it was what GitHub had. The line def write_program(code) was changed to def write_program(program).

Anyway, fixed that, I think I've got this working, just a few minutes more. The change is to RemoteInterface#readline

to this:

def readline(prompt)
  puts(prompt)

  result = (input.gets || "continue")
  result.chomp
rescue Errno::EPIPE, Errno::ECONNABORTED
  "continue"
end

Errno::EPIPE is raised by trunk, Errno::ECONNABORTED is raised by 2.4 and lower.

Thoughts? trying on my fork shortly... Thanks, Greg

deivid-rodriguez · 2018-01-06T19:08:17Z

I found what the issue was with my fork passing the one test. I dl'd a diff of the fork, and (oddly) one line was different. I even checked my browser history, and timestamps matched, so it was what GitHub had. The line def write_program(code) was changed to def write_program(program).

Yeah, sorry! I realized that was incorrect and fixed it here, leaving your fork out of date indeed. Feel free to just work on this same branch instead of your fork, I'll be carefull with force-pushing :)

Your fix looks good, but I think the rescue should be moved up to the confirm and read_command methods (which internally call readline), similarly to puts and print. Otherwise I think the exception could be raised outside of the rescue block you're adding. What do you think?

MSP-Greg · 2018-01-06T19:28:57Z

Yeah, sorry!

That was before I got changed to 'collaborator', so I grabbed the changes and was working just in my fork in a different new branch. IOW, no problem.

I'll be carefull with force-pushing :)

I do that alot also. I'll try to restrict it to things I do before I push here.

I think the rescue should be moved up to the confirm and read_command methods (which internally call readline), similarly to puts and print. Otherwise I think the exception could be raised outside of the rescue block you're adding. What do you think?

Well, it (as listed) just passed on my fork with Appveyor. The commit is here.

I haven't reviewed the remote code enough to know if a particular error should trigger a 'is the client dead?' check,..

Lastly, can we please remove the 'stop on failure' from Appveyor? I normally do all of my work with trunk. I was passing locally, pushed to Appveyor, and the build stopped on 2.3 (which lead to my adding Errno::ECONNABORTED). I've done a lot of AV (Appveyor) testing,and there have been times where local passed and AV failed. Usually timing/resource issues...

deivid-rodriguez · 2018-01-06T19:36:59Z

Well, it (as listed) just passed on my fork with Appveyor. The commit is here.

Yeah, we can't know where the exception will be raised, and most of the times it will happen under the rescue block, but my alternative should be safer I think.

Lastly, can we please remove the 'stop on failure' from Appveyor? I normally do all of my work with trunk. I was passing locally, pushed to Appveyor, and the build stopped on 2.3 (which lead to my adding Errno::ECONNABORTED).

Ok, since you are the only one here working on Windows, let's do whatever works best for you :)

MSP-Greg · 2018-01-06T20:07:54Z

Yeah, we can't know where the exception will be raised, and most of the times it will happen under the rescue block, but my alternative should be safer I think.

I'll defer to you. I'm just starting at the tests and moving inward. You know the whole thing.

Changed. Test passed. Commit is here. I changed the readline code to remove the result variable. Old habit, I prefer to never use local variables if not needed, because 6 months from now, I'd feel a need to double check if result was an attribute I'd forgotten about. I think that made sense....

Let me know if it's good, and I'll push it. then I'll move on to #416...

deivid-rodriguez · 2018-01-06T20:14:55Z

Changed. Test passed. Commit is here.

Yep, that's what I meant. However, the confirm part should probably default to false, not "continue" since super returns a boolean. That said, that part is untested at the moment, so you can just leave it alone for now. So I'd say no change at all there, or rescue and return false, up to you!

I changed the readline code to remove the result variable. Old habit, I prefer to never use local variables if not needed, because 6 months from now, I'd feel a need to double check if result was an attribute I'd forgotten about. I think that made sense..

Mmmm, not sure. I think local variables have preference over attributes, if you were to set an attribute, you'd have to do self.result =, I think? The new code is more concise but unnecessarily chomps "continue". So I'm "meh" on the change, but let's do it if you prefer it that way.

MSP-Greg · 2018-01-06T20:25:32Z

rescue and return false, up to you!

Sorry I missed that. Changed.

unnecessarily chomps "continue".

Can't chomp nil though...

Pushing shortly. Thanks.

deivid-rodriguez · 2018-01-06T20:30:58Z

Sorry I missed that. Changed.

👍

unnecessarily chomps "continue".

Can't chomp nil though...

I mean that, correctly but unnecessarily, calls "continue".chomp (which returns "continue" again) when input.gets is nil. I do like its simplicity though, let's do it! 👍

MSP-Greg · 2018-01-06T20:38:40Z

Cool. Sorry, I meant that input.gets.chomp || "continue" could error.

Thanks for cancelling Appveyor...

deivid-rodriguez · 2018-01-06T20:41:17Z

Cool. Sorry, I meant that input.gets.chomp || "continue" could error.

Yup, that could fail. It was your way or mine, yours works :)

Thanks for cancelling Appveyor...

No problem.

I'll give this a final look tomorrow, hopefully I can add coverage for a couple more cases!

In the case of remote debugging, we don't want to close the interface after each repl. Introduced by e42078f. Adding initial tests for remote debugging.

Since, for example, running `continue` stops the debugger for performance if there's no active breakpoints or catchpoints.

Causing remote debugging to be very slow.

Sometimes tests time out on Linux too.

deivid-rodriguez · 2018-01-13T00:59:42Z

lib/byebug/interfaces/remote_interface.rb

+
+    def print(message)
+      super(message)
+    rescue Errno::EPIPE


@MSP-Greg Should we rescue Errno::ECONNABORTED here and in puts as well? You were seeing that exception on Windows, but only under some versions if I recall correctly?

I don't recall, I'll have to check.

EDIT: I can't find anything re Errno::ECONNABORTED, and I looked thru all the AV builds, both here and my fork. Can't see how it would hurt, especially with a remote.

Totally different subject, the exe/byebug script can be run under windows with another file, but ideally the file contents (one line) should be dependent on whether the gem is installed normally or with a user-install option. Somewhat ties into a PR/issue open in Rubygems right now...

You mentioned it here. I think I'll add Errno::ECONNABORTED in the other methods as well then.

Re exe/byebug, feel free to suggest any changes you want in a separate PR!

You mentioned it here.

Sorry, rather distracted with 'non code' things yesterday evening.

Travis has had large Linux & MacOS backlogs recently (separate occasions). Hope they get it sorted out...

deivid-rodriguez · 2018-01-13T01:00:32Z

I'll merge this as soon as CI passes! I added a small question / comment but we could merge without it.

deivid-rodriguez · 2018-01-25T00:54:01Z

Let's do this!

This test case was introduced as a regression test of deivid-rodriguez#274 in deivid-rodriguez#406. The description of issues/274 says an exception will be raised if the byebug CLI is terminated when main program is sleeping. Before this commit there are 2 breakpoints (`byebug` method call) before `sleep 3` and `"cont"` is called once because the arugment of `remote_debug_connect_and_interrupt` is `"cont"`. I think this is not intended. And the method name of test case is `program_with_two_breakpoints`, on the other hand the program had 3 breakpoints (`byebug`).

This test case was introduced as a regression test of #274 in #406. The description of issues/274 says an exception will be raised if the byebug CLI is terminated when main program is sleeping. Before this commit there are 2 breakpoints (`byebug` method call) before `sleep 3` and `"cont"` is called once because the arugment of `remote_debug_connect_and_interrupt` is `"cont"`. I think this is not intended. And the method name of test case is `program_with_two_breakpoints`, on the other hand the program had 3 breakpoints (`byebug`).

deivid-rodriguez force-pushed the fix/remote_debugging branch 7 times, most recently from ebeea6a to 182828f Compare January 4, 2018 21:13

deivid-rodriguez force-pushed the fix/remote_debugging branch from 182828f to e545240 Compare January 4, 2018 23:43

MSP-Greg force-pushed the fix/remote_debugging branch 2 times, most recently from b9239e7 to 7bddb3e Compare January 5, 2018 19:54

deivid-rodriguez and others added 18 commits January 12, 2018 21:22

Extract server to a separate class

2b08eee

Extract a method

15befcb

Fix remote debugging regression

e7279ed

In the case of remote debugging, we don't want to close the interface after each repl. Introduced by e42078f. Adding initial tests for remote debugging.

Don't crash when client disconnects

7497035

Clarify a comment

cfc5cff

Disable all rubocop metrics

3e8ad87

Shortcut for starting a remote debugger

f75e7ac

Test server & control threads are ignored

ceebb4d

Make sure byebug is started before interrupting it

d49fe2c

Since, for example, running `continue` stops the debugger for performance if there's no active breakpoints or catchpoints.

Remove unnecessary Byebug.start

3dc1e79

Causing remote debugging to be very slow.

Log commands in CI script

9ea4837

See if greater timeout works on Windows

cafc309

remote_test.rb - windows - add -I ruby parameter, change fork to thread

e38b585

Update interfaces/remote_interface.rb for Windows

4ee6d45

Refactor

223ea91

Further refactor

0182286

Increase and normalize timeouts

dd7548f

Sometimes tests time out on Linux too.

Fix windows check

8359dfe

deivid-rodriguez force-pushed the fix/remote_debugging branch from c1e2e2e to 8359dfe Compare January 13, 2018 00:22

deivid-rodriguez commented Jan 13, 2018

View reviewed changes

Rescue the same stuff everywhere

62fbd43

deivid-rodriguez merged commit 1765497 into master Jan 25, 2018

deivid-rodriguez deleted the fix/remote_debugging branch January 25, 2018 00:54

This was referenced Jan 25, 2018

Tests randomly hang #428

Closed

Can't start remote debug server undefined method `wait_connection=' for Byebug:Module (NoMethodError) #185

Closed

yui-knk mentioned this pull request Dec 8, 2018

Make program_with_two_breakpoints has exactly 2 breakpoints #517

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Love for remote debugging #406

Love for remote debugging #406

deivid-rodriguez commented Dec 31, 2017 •

edited

Loading

deivid-rodriguez commented Jan 4, 2018

MSP-Greg commented Jan 4, 2018

deivid-rodriguez commented Jan 4, 2018

MSP-Greg commented Jan 5, 2018 •

edited

Loading

MSP-Greg commented Jan 5, 2018

deivid-rodriguez commented Jan 5, 2018

MSP-Greg commented Jan 5, 2018

MSP-Greg commented Jan 5, 2018

deivid-rodriguez commented Jan 5, 2018

MSP-Greg commented Jan 5, 2018

MSP-Greg commented Jan 6, 2018 •

edited

Loading

deivid-rodriguez commented Jan 6, 2018 •

edited

Loading

MSP-Greg commented Jan 6, 2018 •

edited

Loading

deivid-rodriguez commented Jan 6, 2018

MSP-Greg commented Jan 6, 2018

deivid-rodriguez commented Jan 6, 2018

MSP-Greg commented Jan 6, 2018

deivid-rodriguez commented Jan 6, 2018

MSP-Greg commented Jan 6, 2018

deivid-rodriguez commented Jan 6, 2018 •

edited

Loading

deivid-rodriguez Jan 13, 2018

MSP-Greg Jan 13, 2018 •

edited

Loading

deivid-rodriguez Jan 13, 2018

MSP-Greg Jan 13, 2018 •

edited

Loading

deivid-rodriguez commented Jan 13, 2018

deivid-rodriguez commented Jan 25, 2018

Love for remote debugging #406

Love for remote debugging #406

Conversation

deivid-rodriguez commented Dec 31, 2017 • edited Loading

deivid-rodriguez commented Jan 4, 2018

MSP-Greg commented Jan 4, 2018

deivid-rodriguez commented Jan 4, 2018

MSP-Greg commented Jan 5, 2018 • edited Loading

MSP-Greg commented Jan 5, 2018

deivid-rodriguez commented Jan 5, 2018

MSP-Greg commented Jan 5, 2018

MSP-Greg commented Jan 5, 2018

deivid-rodriguez commented Jan 5, 2018

MSP-Greg commented Jan 5, 2018

MSP-Greg commented Jan 6, 2018 • edited Loading

deivid-rodriguez commented Jan 6, 2018 • edited Loading

MSP-Greg commented Jan 6, 2018 • edited Loading

deivid-rodriguez commented Jan 6, 2018

MSP-Greg commented Jan 6, 2018

deivid-rodriguez commented Jan 6, 2018

MSP-Greg commented Jan 6, 2018

deivid-rodriguez commented Jan 6, 2018

MSP-Greg commented Jan 6, 2018

deivid-rodriguez commented Jan 6, 2018 • edited Loading

deivid-rodriguez Jan 13, 2018

Choose a reason for hiding this comment

MSP-Greg Jan 13, 2018 • edited Loading

Choose a reason for hiding this comment

deivid-rodriguez Jan 13, 2018

Choose a reason for hiding this comment

MSP-Greg Jan 13, 2018 • edited Loading

Choose a reason for hiding this comment

deivid-rodriguez commented Jan 13, 2018

deivid-rodriguez commented Jan 25, 2018

deivid-rodriguez commented Dec 31, 2017 •

edited

Loading

MSP-Greg commented Jan 5, 2018 •

edited

Loading

MSP-Greg commented Jan 6, 2018 •

edited

Loading

deivid-rodriguez commented Jan 6, 2018 •

edited

Loading

MSP-Greg commented Jan 6, 2018 •

edited

Loading

deivid-rodriguez commented Jan 6, 2018 •

edited

Loading

MSP-Greg Jan 13, 2018 •

edited

Loading

MSP-Greg Jan 13, 2018 •

edited

Loading