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

avoid hang in stringValue #2243

Merged
merged 7 commits into from Aug 4, 2016
Merged

avoid hang in stringValue #2243

merged 7 commits into from Aug 4, 2016

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Jul 27, 2016

I could never induce splitObjects to call children, even following the steps in #2242, but the potential for an infinite loop was undeniable. Good catch, @pjrobertson!

This should prevent such a loop. The second commit may or may not help as well, but at worst, it makes things more efficient.

skurfer added 2 commits Jul 27, 2016
splitObjects tries three things: check if count is 1, check for object components, get children. We already know the count is not one, and getting children (if it doesn't cause a loop) just splits the string on \n, which we would only reverse here. So go straight for the object components.

fixes #2242
stringValue is a bit heavy handed. We know the text is present since we test for it, and we know that's what stringValue will end up returning (after doing many tests).
@pjrobertson
Copy link
Member

pjrobertson commented Jul 27, 2016

I like the 2nd commit, even though it is probably still a bit hacky. Since what you said about us storing identifiers for text objects is no longer the case, can it not just be removed in its entirety?

It seems a bit 'black magic' to me!

@skurfer
Copy link
Member Author

skurfer commented Jul 28, 2016

Since what you said about us storing identifiers for text objects is no longer the case, can it not just be removed in its entirety?

I found the reason for that code. See #1858. Sounds like the identifiers were getting stored as text, but they referred to more complicated objects. Makes more sense than text objects with identifiers surviving in someone’s indexes all this time. I’m still not sure if it’s safe to remove, though. I can look deeper tomorrow with fresh eyes.

// get the string value for each object in the collection
stringValue = [[[self splitObjects] arrayByPerformingSelector:@selector(stringValue)] componentsJoinedByString:@"\n"];
stringValue = [[[obj objectForCache:kQSObjectComponents] arrayByPerformingSelector:@selector(stringValue)] componentsJoinedByString:@"\n"];
Copy link
Member

@pjrobertson pjrobertson Jul 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so will there ever be a case that the kQSObjectComponents won't have been set yet when you get here?

Copy link
Member Author

@skurfer skurfer Jul 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn’t get it into that state, but it must be possible, otherwise children would have never been called when we were using splitObjects as described in your report of the issue.

If it’s not set, it moves to the next thing. No big deal, since this entire section didn’t exist a year ago.

Copy link
Member

@pjrobertson pjrobertson Aug 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, what you added is an improvement, and if it's not there it'd be "how it always was"

I'm going to merge but then add a comment RE this isn't ideal - incase it catches us out one day

pjrobertson added 2 commits Aug 3, 2016
(and why we didn't use `[obj splitObjects]`. More info in the commit message of c43577d
@pjrobertson
Copy link
Member

pjrobertson commented Aug 3, 2016

OK I pushed a small change to add a comment to your change. I'm happy with this as is (even if it is slightly hacky).
I also added some multiline string tests. Perhaps controversially, the new tests I added fail... because a multiline string of paths (multiple file objects) return 2 Folders in "XXX".

Now the question is: should the stringValue be 2 Folders in "XXX" or is that for displayName right? I'd say the string value (useful for pasting etc.) should be the paths split up using \n, but the display name should be the user friendly description.

If this is true, then either this usage of stringValue was done incorrectly or we're using stringValue instead of displayName somewhere

@pjrobertson
Copy link
Member

pjrobertson commented Aug 3, 2016

Oh, wait... the reason it's failing is because

  1. There are no handlers set for file objects (because QSReg hasn't been populated when the tests run)
  2. Even if the handler were set, because we're not calling [obj splitObjects] anymore (to avoid the crash this PR was fixing) we're getting nil from the cache.

Now we have the eternal dilemma of tests failing because they don't represent the real world... because the real world is way too complicated!

pjrobertson added 3 commits Aug 3, 2016
In unit tests, calling QSGetApplicationSupportFolder() might return nil and cause a crash - because QSApplicationSupportPath was never set.
These changes make sure it's always set (and in fact stop QSApplicationSupportPath from being extern
@pjrobertson
Copy link
Member

pjrobertson commented Aug 4, 2016

OK, let's try and get this merged so we're not sidetracked and discussing something else for ever! I've commented out the problematic unit tests, to be revised at a later date

@skurfer
Copy link
Member Author

skurfer commented Aug 4, 2016

OK, let's try and get this merged so we're not sidetracked and discussing something else for ever! I've commented out the problematic unit tests, to be revised at a later date

Yes, thank you! It’s supposed to be a stupid one-line change. 😃

@skurfer skurfer merged commit b8907c2 into master Aug 4, 2016
2 checks passed
@skurfer skurfer deleted the string-hang branch Aug 4, 2016
skurfer added a commit that referenced this issue Aug 4, 2016
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

3 participants