tzset(3) calls before new localtime_r(3) calls

[Tom Ryder]

Hello Vim developers; attached is a suggested patch for correcting
a problem I noticed with time zones and Vim's strftime() function since
v8.1.1313, with an accompanying test.

I was not sure how best to do this portably, but I hope that the patch
description and the diff that corrects the problem on my Debian
GNU/Linux system at least points you in the right direction for
correcting the issue.

--
Tom Ryder <https://sanctum.geek.nz/>

[Bram Moolenaar]

[...]

> The POSIX spec for localtime(3) and localtime_r(3) says:
>
> > The localtime() function converts the calendar time timep to
> > broken-down time representation, expressed relative to the user's
> > specified timezone. The function acts as if it called tzset(3) and
> > sets the external variables tzname with information about the current
> > timezone, timezone with the difference between Coordinated Universal
> > Time (UTC) and local standard time in seconds, and daylight to
> > a nonzero value if daylight savings time rules apply during some part
> > of the year. The return value points to a statically allocated struct
> > which might be overwritten by subsequent calls to any of the date and
> > time functions. The localtime_r() function does the same, but stores
> > the data in a user-supplied struct. It need not set tzname, timezone,
> > and daylight.
>
> The last sentence of the specification is most relevant here. Patch
> 8.1.1313 (tag v8.1.1313, commit 63d2555) replaces calls to localtime(3)
> with localtime_r(3) if available, but does not call tzset() before each
> invocation.

I don't understand the sentence. "It need not set", what does that mean
exactly? That it's not needed to set to variables? That it maybe sets
them, maybe not?

Anyway, who needs those variables anyway? The manpage just mentions
they are set, but not where or how they are used.

I would guess that localtime() always sets the variables, while
localtime_r() is not guaranteed to do that. But it does NOT say that
calling tzset() before localtime_r() is required to get correct results.

> On Debian GNU/Linux with GNU libc 2.24-11+deb9u4, this results in
> a timezone "sticking" after the first tzset() instance, meaning that
> changes to the TZ environment variable do not take effect in between
> invocations.
>
> As an example:
>
> :let $TZ = "UTC" | echo $TZ.' -> '.strftime("%c")
> UTC -> Wed 12 Jun 2019 04:10:29 UTC
> :let $TZ = "Pacific/Auckland" | echo $TZ.' -> '.strftime("%c")
> Pacific/Auckland -> Wed 12 Jun 2019 04:10:32 UTC
>
> Note that neither the hour nor the reported timezone changes in the
> stftime() output between invocations, even though the value of the TZ
> environment value itself does change as expected.
>
> This commit adds a call to tzset(3) before each call to localtime_r(3),
> and corrects the above misbehavior, including checks for the
> availability of both functions.

How expensive is a call to tzset()? It looks like it may read the
timezone database, which means file I/O and thus quite expensive.
Perhaps tzset() should only be called once, or perhaps only when the $TZ
variable was changed? I actually don't see how $TZ can change while Vim
is running, would require starting Vim on a laptop, travelling to
another timezone, and continuing editing?


--
hundred-and-one symptoms of being an internet addict:
161. You get up before the sun rises to check your e-mail, and you
find yourself in the very same chair long after the sun has set.

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[Tom Ryder]

Hi Bram, thanks for looking at the issue.

On Wed, Jun 12, 2019 at 07:34:36PM +0200, Bram Moolenaar wrote:
>I would guess that localtime() always sets the variables, while
>localtime_r() is not guaranteed to do that. But it does NOT say that
>calling tzset() before localtime_r() is required to get correct
>results.

That is true. I am reporting the issue with a patch that worked for me,
and quoting from what seemed to me to be the relevant part of the spec.

I don't know if this is the best way to restore the behaviour I want.
Similar to the example code in the patch, I had been setting and
restoring the $TZ variable within Vim to set the time zone for a call to
strftime(), in order to get UTC timestamps. Restoring the previous time
zone afterwards worked correctly until the localtime_r patch. After
that patch, it no longer worked. With a call to tzset() added before
those calls, per my patch, it works again.

>Perhaps tzset() should only be called once, or perhaps only when the
>$TZ variable was changed?

The latter idea seems good to me. Perhaps even a new function
settimezone() could be added, or a new optional parameter for
strftime()?

[Bram Moolenaar]

My current setup doesn't even have $TZ set. Thus calling tzset() every
time would just add overhead.

Can you make a patch that adds a function to call localtime_r() and
keeps the current value of $TZ, and calls tzset() only when it changes?

--
"Hit any key to continue" is very confusing when you have two keyboards.

[Tom Ryder]

On Thu, Jun 13, 2019 at 08:40:27PM +0200, Bram Moolenaar wrote:
>Can you make a patch that adds a function to call localtime_r() and
>keeps the current value of $TZ, and calls tzset() only when it changes?

Yes, I think so. I'll attempt that later today. Thanks.

--
Tom Ryder <https://sanctum.geek.nz/>

The next 1<<10 years are ours.

[Tom Ryder]

On Fri, Jun 14, 2019 at 12:43:32PM +1200, Tom Ryder via vim_dev wrote:
>On Thu, Jun 13, 2019 at 08:40:27PM +0200, Bram Moolenaar wrote:
>>Can you make a patch that adds a function to call localtime_r() and
>>keeps the current value of $TZ, and calls tzset() only when it
>>changes?
>
>Yes, I think so. I'll attempt that later today. Thanks.

Attached is another patch attempting this. I'm happy to make any
required changes.

[Dominique Pellé]

The code tzset is here:
https://github.com/eggert/tz/blob/master/localtime.c#L1396

tzset() looks fast when TZ has not changed as from what I see,
it then merely:

* locks a mutex
* calls getenv("TZ")
* compares TZ value with a previous value
* unlocks a mutex

Regards
Dominique

[Bram Moolenaar]

Thanks.

I rather not add a new file, this can go into evalfunc.c. That file is
getting a bit big, I'll probably move some things out of there later.

> + if (strncmp(tz, tz_cache, DATETIME_TZ_CACHEL) != 0)
> + {
> + if (p_verbose > 0)
> + {
> + verbose_enter();
> + smsg(
> + _("Updating cached timezone TZ from environment"));
> + verbose_leave();
> + }

I don't think we should do this. This is only useful for debugging, not
really for users.

--
hundred-and-one symptoms of being an internet addict:

187. You promise yourself that you'll only stay online for another
15 minutes...at least once every hour.

[Bram Moolenaar]

That is one implementation. How about other systems?
I think we should stay on the safe side.

--
hundred-and-one symptoms of being an internet addict:

186. You overstay in the office so you can have more time surfing the net.

[Tom Ryder]

On Sat, Jun 15, 2019 at 04:06:17PM +0200, Bram Moolenaar wrote:
>I rather not add a new file, this can go into evalfunc.c. That file is
>getting a bit big, I'll probably move some things out of there later.

I had considered that, but localtime()/localtime_r() is called in two
other contexts outside evalfunc.c:f_strftime(): memline.c:get_ctime(),
which is called by swapfile_info() and attention_message() in the same
file, and undo.c:u_add_time(), so I wasn't sure how to make that work.

I was going to add the vim_localtime() prototype to vim.h, but that
seemed wrong because there are so few existing ones already in there,
although a few of the ones towards the end of the file are time-related.

>I don't think we should do this. This is only useful for debugging,
>not really for users.

OK, agreed. Thanks.

[Bram Moolenaar]

Tom Ryder wrote:

> On Sat, Jun 15, 2019 at 04:06:17PM +0200, Bram Moolenaar wrote:
> >I rather not add a new file, this can go into evalfunc.c. That file is
> >getting a bit big, I'll probably move some things out of there later.
>
> I had considered that, but localtime()/localtime_r() is called in two
> other contexts outside evalfunc.c:f_strftime(): memline.c:get_ctime(),
> which is called by swapfile_info() and attention_message() in the same
> file, and undo.c:u_add_time(), so I wasn't sure how to make that work.
>
> I was going to add the vim_localtime() prototype to vim.h, but that
> seemed wrong because there are so few existing ones already in there,
> although a few of the ones towards the end of the file are time-related.

Just add it as a global function, without "static". Then update the
prototype in src/proto/evalfunc.pro. You can do it automatically if you
have cproto, but it's easy enough to do manually.

Sorry you had to update all those files when adding a new source file,
that's why I don't like adding many smaller files. Well, and it makes
building slower.

--
hundred-and-one symptoms of being an internet addict:

198. You read all the quotes at Netaholics Anonymous and keep thinking
"What's wrong with that?"

[Tom Ryder]

On Sun, Jun 16, 2019 at 03:32:30PM +0200, Bram Moolenaar wrote:
>Just add it as a global function, without "static". Then update the
>prototype in src/proto/evalfunc.pro. You can do it automatically if
>you have cproto, but it's easy enough to do manually.

OK, I will do that.

>Sorry you had to update all those files when adding a new source file,
>that's why I don't like adding many smaller files. Well, and it makes
>building slower.

That's OK; it was interesting to figure out. Might be handy for me in
future too.

[Tom Ryder]

On Mon, Jun 17, 2019 at 09:47:35AM +1200, Tom Ryder wrote:
>On Sun, Jun 16, 2019 at 03:32:30PM +0200, Bram Moolenaar wrote:
>>Just add it as a global function, without "static". Then update the
>>prototype in src/proto/evalfunc.pro. You can do it automatically if
>>you have cproto, but it's easy enough to do manually.
>
>OK, I will do that.

Hi Bram; is the attached patch any closer? It builds with --features
set to "tiny" or "normal" to test the +eval conditionals, passes all
tests on both, and corrects the issue for Vim's strftime() on the
latter. I appreciate your patience on this.