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

Regex to remove comments clashing with file globbing - causing hanging process #110

Closed
jackbrewer opened this issue Jun 9, 2015 · 16 comments
Closed
Labels

Comments

@jackbrewer
Copy link
Contributor

@jackbrewer jackbrewer commented Jun 9, 2015

Problem

I've run in to an issue when using Stylus file globbing for @import and @require.

Stylint process can hang indefinitely, using 100% CPU (depending on file length, and the location of the glob within the file).

Example:
@require 'components/*'

Diagnosis

It seems the regex in parse.js is catching the trailing /* of the above line and is treating it as an opening comment. As it can never find a closing comment, it keeps searching through subsequent lines, getting exponentially slower. Multiple globs in a file makes things worse.

The following gif shows a stripped back example - you can see the 'steps' in the top-right ~doubling with each new-line.

runaway-regex

Live Example - https://regex101.com/r/jU0eJ2/1

Solution

New regex which accounts for this edge-case. I know @benedfit is taking a look at possible fix, but we're finding it hard to find a solution which doesn't introduce another edge-case where an actual comment gets missed, so I thought I'd raise here in the meantime.

@benedfit
Copy link
Contributor

@benedfit benedfit commented Jun 9, 2015

I'm playing around with a Regex that might fix this problem: https://regex101.com/r/zM0xA6/, and including some extra test cases that might not have been previously tested against. I'll update you with my progress

@benedfit
Copy link
Contributor

@benedfit benedfit commented Jun 9, 2015

@rossPatton https://regex101.com/r/zM0xA6/ is close, but it wouldn't remove comments in the format /*'word'*/

@benedfit
Copy link
Contributor

@benedfit benedfit commented Jun 9, 2015

https://regex101.com/r/zM0xA6/6 has cracked it. Going to submit a PR shortly

Shout-out to @kuba81 and his other-worldly regex mastery

@rossPatton
Copy link
Collaborator

@rossPatton rossPatton commented Jun 9, 2015

Weird, i've never hit this issue personally (and I use globs when I require) but it's awesome to see that it's already been resolved!

@rossPatton rossPatton added the bug label Jun 9, 2015
@jackbrewer
Copy link
Contributor Author

@jackbrewer jackbrewer commented Jun 10, 2015

Yeah, it's a strange one. File length definitely plays a part. Our index stylus file (mainly just @requires) is well commented/documented so is 100 lines+. It was only here that we've spotted the issue.

@rossPatton
Copy link
Collaborator

@rossPatton rossPatton commented Jun 10, 2015

Interesting, I'll try to merge it in tonight.

@rossPatton
Copy link
Collaborator

@rossPatton rossPatton commented Jun 11, 2015

@benedfit @jackbrewer update on this, i took a look at the pr, tests passed, looked okay, merged it into develop, but after messing with it more it seems to break things.

Playing around with it in regexr/regex101, it doesn't seem to match the same way the old regex matched.

I ended up tweaking the existing one, which seems to solve the problem: see here

It's on develop now, if you want to pull and take a look. I'll push it up to npm with confirmation that it solves the issue.

@benedfit
Copy link
Contributor

@benedfit benedfit commented Jun 11, 2015

@rossPatton just took a look; the new regex will ignore comments that start on the same line as an other Stylus, see https://regex101.com/r/zY7fR9/3. Is that the desired outcome?

@jackbrewer
Copy link
Contributor Author

@jackbrewer jackbrewer commented Jun 11, 2015

Tested develop and all seems to be working well. Happy to test again based on the outcome of the above comment ^

@rossPatton
Copy link
Collaborator

@rossPatton rossPatton commented Jun 11, 2015

@rossPatton
Copy link
Collaborator

@rossPatton rossPatton commented Jun 11, 2015

I've also pushed this change up to develop, if you want to test that way.

@rossPatton
Copy link
Collaborator

@rossPatton rossPatton commented Jun 12, 2015

@benedfit
Copy link
Contributor

@benedfit benedfit commented Jun 15, 2015

@rossPatton tested it and it works well

@rossPatton
Copy link
Collaborator

@rossPatton rossPatton commented Jun 15, 2015

Excellent!

@jackbrewer
Copy link
Contributor Author

@jackbrewer jackbrewer commented Jun 16, 2015

👍 Tested new develop changes and still fixed and working as expected.

@rossPatton
Copy link
Collaborator

@rossPatton rossPatton commented Jun 19, 2015

0.9.10 fixes this issue, sorry it took so long

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

No branches or pull requests

3 participants