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

feat: fix pathways to allow debugging hot-transpiled modules #174

Merged
merged 8 commits into from
Jan 6, 2020

Conversation

connor4312
Copy link
Member

Previously, this extension fared poorly if one attempted to debug a Node
script that had in-place transpilation going on (e.g. @babel/register or
ts-node). There were a few reasons for this.

The most trivial one is that we didn't do content hashing for Node
script as a performance optimization. Unfortuantely, we can't assume
that any file on disk is actually what we're running in this scenario :)

A second trivial one is that the source map filtering we added didn't
support source map data URIs. We added a filter to ignore source maps
coming from node_modules or outside the workspace folder, and we just
didn't have logic for matching data URIs there. Now, if we see a data
URI, we'll just treat the embedding file as the source map location for
comparison purposes.

The bulk of this commit deals with the third change. By default, we
eagerly throw out breakpoints that match lines the exact file path or
URI. This becomes problematic because the transpilation happens
in-place, generating compiled sources that live appear at the same paths
as the source files. The approach we take here is that if we see a
source map which lands breakpoints in a file for which we've previously
set breakpoints by URL, then remove the URL-set breakpoints in favor of
the sourcemapped breakpoints.

The bulk of changes were around resiliency for this, since breakpoints
become a little more dynamic now in some cases:

  • We store a list of 'applying' breakpoints in addition to 'applied'
    ones, which we're able to remove or deadletter (e.g. if we run into a
    sourcemap while still setting a URL BP)
  • We used to store only a single UI location for the breakpoint, we now
    keep track of them and will change the location if a breakpoint is
    moved or adjusted.
  • Fixed a (probably rare) case where a BP could leak if a the thread
    changed while the breakpoint was being applied.

Fixes #127

Previously, this extension fared poorly if one attempted to debug a Node
script that had in-place transpilation going on (e.g. @babel/register or
ts-node). There were a few reasons for this.

The most trivial one is that we didn't do content hashing for Node
script as a performance optimization. Unfortuantely, we can't assume
that any file on disk is actually what we're running in this scenario :)

A second trivial one is that the source map filtering we added didn't
support source map data URIs. We added a filter to ignore source maps
coming from node_modules or outside the workspace folder, and we just
didn't have logic for matching data URIs there. Now, if we see a data
URI, we'll just treat the embedding file as the source map location for
comparison purposes.

The bulk of this commit deals with the third change. By default, we
eagerly throw out breakpoints that match lines the exact file path or
URI. This becomes problematic because the transpilation happens
in-place, generating compiled sources that live appear at the same paths
as the source files. The approach we take here is that if we see a
source map which lands breakpoints in a file for which we've previously
set breakpoints by URL, then remove the URL-set breakpoints in favor of
the sourcemapped breakpoints.

The bulk of changes were around resiliency for this, since breakpoints
become a little more dynamic now in some cases:
 - We store a list of 'applying' breakpoints in addition to 'applied'
   ones, which we're able to remove or deadletter (e.g. if we run into a
   sourcemap while still setting a URL BP)
 - We used to store only a single UI location for the breakpoint, we now
   keep track of them and will change the location if a breakpoint is
   moved or adjusted.
 - Fixed a (probably rare) case where a BP could leak if a the thread
   changed while the breakpoint was being applied.
@dgozman
Copy link
Contributor

dgozman commented Dec 13, 2019

Hmm... The whole idea of transpiler transforming file in place sounds terrible. This means we loose the ability to debug either the original file, or the transformed one. Can we engage with @babel/register and/or other projects which do this kind of thing? IIUC, the easy fix for them would be to additionally include sourceUrl in the generated file which points to a different location, so that we don't even map generated file to non-matching source file. WDYT?

@connor4312
Copy link
Member Author

I don't disagree, it is somewhat terrible, but it's been common practice in Node since the first transpilers were born...

I'll prototype and follow up with the maintainers of common tools in a few days, but this'll be a scenario we need to support for a good while regardless.

Following Rob's suggestion of what the v1 adapter does. When we set
a breakpoint in a file, we'll also set a breakpoint on the first line
that we consider to be an instrumentation breakpoint. We treat like
the normal `beforeScriptWithSourceMapExecution` breakpoint, waiting
breakpoints to be set (which in the case of babel/ts-node, includes any
necessary adjustment) before continuing on.

This commit also makes a tweak (over the existing changes in the PR)
to avoid mapping breakpoints by path if we see our source is mapped
in-place. The check for this is rather ugly, but it works.
@connor4312 connor4312 force-pushed the feat/in-place-breakpoints branch from b279e29 to b566724 Compare December 17, 2019 18:07
@connor4312 connor4312 force-pushed the feat/in-place-breakpoints branch from b566724 to 4eac739 Compare December 17, 2019 18:15
@connor4312 connor4312 force-pushed the feat/in-place-breakpoints branch from 1ba0747 to 11e025b Compare December 20, 2019 00:48
@connor4312 connor4312 force-pushed the feat/in-place-breakpoints branch from 11e025b to dcf125d Compare December 20, 2019 00:49
@connor4312
Copy link
Member Author

Test flake seems unrelated

connor4312 added a commit that referenced this pull request Dec 20, 2019
First of all, apologies for the churn. While this change is relatively
simple, I realized that hit condition breakpoints further diverges the
user-defined breakpoints and the managed 'entrypoint' breakpoints (as
of #174), where we don't have hit conditions or DAP representations.

Therefore, there's now a base "Breakpoint" class which implements the
purely CDP-side of things, which a `UserDefinedBreakpoint` extends with
metadata around DAP, and the `EntryBreakpoint` extends in a minimal way.
After doing the split there was almost no changes the existing code
around the breakpoints, which is good evidence that we are indeed
dealing with two different types of things!

That done, there's a new `HitBreakpoint` class whose parsing logic is
copied from the existing debug adapters. The UserDefinedBreakpoint
optionally includes a HitBreakpoint, and we check on these when we hit
a location to see whether we should automatically continue, or not.

This works in my quick manual tests. Haven't had a chance to write unit
tests for this, but I wanted to get it in PR before I head out for the
holidays.
connor4312 added a commit that referenced this pull request Dec 20, 2019
First of all, apologies for the churn. While this change is relatively
simple, I realized that hit condition breakpoints further diverges the
user-defined breakpoints and the managed 'entrypoint' breakpoints (as
of #174), where we don't have hit conditions or DAP representations.

Therefore, there's now a base "Breakpoint" class which implements the
purely CDP-side of things, which a `UserDefinedBreakpoint` extends with
metadata around DAP, and the `EntryBreakpoint` extends in a minimal way.
After doing the split there was almost no changes the existing code
around the breakpoints, which is good evidence that we are indeed
dealing with two different types of things!

That done, there's a new `HitBreakpoint` class whose parsing logic is
copied from the existing debug adapters. The UserDefinedBreakpoint
optionally includes a HitBreakpoint, and we check on these when we hit
a location to see whether we should automatically continue, or not.

This works in my quick manual tests. Haven't had a chance to write unit
tests for this, but I wanted to get it in PR before I head out for the
holidays.
* feat: initial implementation of hit condition breakpoints

First of all, apologies for the churn. While this change is relatively
simple, I realized that hit condition breakpoints further diverges the
user-defined breakpoints and the managed 'entrypoint' breakpoints (as
of #174), where we don't have hit conditions or DAP representations.

Therefore, there's now a base "Breakpoint" class which implements the
purely CDP-side of things, which a `UserDefinedBreakpoint` extends with
metadata around DAP, and the `EntryBreakpoint` extends in a minimal way.
After doing the split there was almost no changes the existing code
around the breakpoints, which is good evidence that we are indeed
dealing with two different types of things!

That done, there's a new `HitBreakpoint` class whose parsing logic is
copied from the existing debug adapters. The UserDefinedBreakpoint
optionally includes a HitBreakpoint, and we check on these when we hit
a location to see whether we should automatically continue, or not.

This works in my quick manual tests. Haven't had a chance to write unit
tests for this, but I wanted to get it in PR before I head out for the
holidays.

* fixup! entrypoint breakpoint behavior

Added fixes and unit tests while looking into #204.

 - Our nice new entrypoint breakpoint interfered with user-defined
   breakpoints. CDP errors if we try to set multiple breakpoints at the
   same locations, so if we see the user has a BP on the first line, we
   omit setting our automatic breakpoint. We also tweak behavior to
   treat any first-line breakpoint as an entrypoint breakpoint.
 - We were unnecessarily deduplicating breakpoint application requests
   which caused breakpoints which resolved to the exact same location in
   transpiled code to not get set.

* fixup! only retry in ci

* fixup! update the test assertions now that offsets are fixed
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.

Sending a sourceReference for a source when it has a properly resolved path
3 participants