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

SR-11917: Using JSONEncoder's .outputFormatting = .sortedKeys has a memory leak on Linux #2655

Merged
merged 1 commit into from Feb 8, 2020

Conversation

spevans
Copy link
Collaborator

@spevans spevans commented Feb 7, 2020

  • Any use of NSString.compare(_:options:range:locale:) with a non-nil locale
    would leak on Linux.

  • _CFCompareStringsWithLocale was leaking the collator on non-macOS
    platforms.

  • Use the collator cache on Linux and Win32 the same way that macOS does
    and for other platforms ensure that any allocated collator is freed.

@spevans
Copy link
Collaborator Author

spevans commented Feb 7, 2020

@swift-ci test

#if TARGET_OS_MAC || TARGET_OS_WIN32 || TARGET_OS_LINUX
#if TARGET_OS_MAC
Copy link
Contributor

Choose a reason for hiding this comment

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

This exposes the variable collator at l616, which is still hidden behind an #if tag above (l577), so this doesn't build properly on other platforms.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for catching this. Do you think the the #if TARGET_OS_MAC || TARGET_OS_WIN32 || TARGET_OS_LINUX could be removed entirely and the code would work on the BSD platforms? btw which platforms are you testing on?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that builds cleanly now.

The platform-specific branches could be removed -- I did a rush check adding TARGET_OS_BSD on every appearance of TARGET_OS_LINUX atop this commit -- but they'd need to be removed in more places than you are changing here, so perhaps that should be done in a separate PR.

However, I haven't run any of the tests; right now I am just checking to ensure things build properly on OpenBSD without error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for checking, yes probably best to support other platforms in a future PR.

…emory leak on Linux

- Any use of NSString.compare(_:options:range:locale:) with a non-nil locale
  would leak on Linux.

- _CFCompareStringsWithLocale was leaking the collator on non-macOS
  platforms.

- Use the collator cache on Linux and Win32 the same way that macOS
  does.
@spevans
Copy link
Collaborator Author

spevans commented Feb 8, 2020

@swift-ci test

@spevans
Copy link
Collaborator Author

spevans commented Feb 8, 2020

@swift-ci test linux

1 similar comment
@spevans
Copy link
Collaborator Author

spevans commented Feb 8, 2020

@swift-ci test linux

@spevans
Copy link
Collaborator Author

spevans commented Feb 8, 2020

@compnerd could you test this on windows when you get a chance? thanks

@spevans
Copy link
Collaborator Author

spevans commented Feb 8, 2020

@swift-ci test linux

1 similar comment
@spevans
Copy link
Collaborator Author

spevans commented Feb 8, 2020

@swift-ci test linux

@spevans
Copy link
Collaborator Author

spevans commented Feb 8, 2020

@swift-ci test and merge

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