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

Fix system timezone memoization. #649

Merged
merged 1 commit into from Mar 5, 2018

Conversation

@QuLogic
Copy link
Contributor

commented Mar 5, 2018

The result of get_system_tz comes from a STRING_ELT, which seems to be automatically garbage collected, causing the static character pointer to point to an invalid location. Duplicating the string is a small leak, but prevents this invalid access.

Alternatively, you could not do the memoization; I'm not sure what the benefit or downside of that is.

This should fix several of the failures on CRAN.

Fix system timezone memoization.
The result of `get_system_tz` comes from a `STRING_ELT`, which seems to
be automatically garbage collected, causing the static character pointer
to point to an invalid location. Duplicating the string is a small leak,
but prevents this invalid access.

@QuLogic QuLogic force-pushed the QuLogic:fix-memo branch from 18e4974 to c463635 Mar 5, 2018

@vspinu

This comment has been minimized.

Copy link
Member

commented Mar 5, 2018

This makes sense. But I think duplication should be better done within get_system_tz. What do you think?

which seems to be automatically garbage collected

Have you actually seen this with 1.7.3? You should see garbage returned by lubridate:::C_local_tz(). It was indeed the case with 1.7.2 but I changed the memoization in 1.7.3 and haven't noticed the problem since.

This should fix several of the failures on CRAN.

Not sure. Empty tz which shows there is not "garbage".

@QuLogic

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2018

But I think duplication should be better done within get_system_tz. What do you think?

I didn't put it there because there's no guarantee of it only being called once, whereas we know the static variable is. Just a slightly lower chance of extra leaking that way.

Have you actually seen this with 1.7.3? You should see garbage returned by lubridate:::C_local_tz(). It was indeed the case with 1.7.2 but I changed the memoization in 1.7.3 and haven't noticed the problem since.

Yes, I've never used 1.7.2. It works the first time, but not on the second (or maybe third or fourth) call due to the pointers changing around.

This should fix several of the failures on CRAN.

Not sure. Empty tz which shows there is not "garbage".

The empty tz is printed out by the outer wrapper; this garbage shows up in some inner frames but is never printed out.

@vspinu

This comment has been minimized.

Copy link
Member

commented Mar 5, 2018

Just a slightly lower chance of extra leaking that way.

Ok. Makes sense.

@vspinu vspinu merged commit 37ae03e into tidyverse:master Mar 5, 2018

3 checks passed

codecov/patch 100% of diff hit (target 73.56%)
Details
codecov/project 73.56% (+0%) compared to 9784231
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@QuLogic QuLogic deleted the QuLogic:fix-memo branch Mar 5, 2018

@shrektan

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

Encountered some strange error on version lubridate v1.7.2 (not be able to build the dev version on Windows because it reports that ld.exe: cannot find -lcctz), like lubridate::force_tz(Sys.time(), "UTC") returns apparently different results occasionally (difficult to reproduce)... Hopefully this PR solves the issue...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.