bugGNU Octave - Bugs: bug #53539, GUI file browser date stamps /...

 
 

bug #53539: GUI file browser date stamps / editor script file appear modified when parsing date strings

Submitter:  Lars Kindermann <larskindermann>
Submitted:  Sun 01 Apr 2018 02:41:59 AM UTC
   
 
Category:  GUI Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Unexpected Error or Warning
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 14 Jan 2019 08:47:22 AM UTC, comment #26: 

Given that glibc is a GNU program, I suppose we could just copy __strptime_internal and make it a function in liboctave, adjusting its time zone behavior to our liking. It's probably pretty stable now so it wouldn't be much of a maintenance burden.

Andrew Janke <apjanke>
Mon 14 Jan 2019 08:06:11 AM UTC, comment #25: 

The recent topic "Problem processing data logs" on help-octave should be considered when developing a solution for this bug. The Unix decision to make the functionality of __strptime_internal hidden was taken when the world of computing was very different than it is now and should be revisited.

Furthermore if the functionality is exposed in the C API it should be exposed in Octave too, to allow a much better solution to "Problem processing data logs" than is currently available.

Ian McCallion <ianmcc>
Mon 14 Jan 2019 12:27:48 AM UTC, comment #24: 

Ouch. Okay, I understand why the code is doing what it does now.

Andrew Janke <apjanke>
Sat 12 Jan 2019 11:36:14 AM UTC, comment #23: 

@Andrew: Setting TZ to UTC0 around strptime() was introduced due to a bug: Converting a string into a tz structure in timezones with daylight saving depends on the current daylight saving state, and gives strange results during the hour where the clocks are turned back. Only in UTC0 strptime() just literally converts the string elements into the fields of a tz structure without fiddling around with daylight saving.

There is no way of asking libc's strptime() directly to use a specific timnezone, it always retrieves TZ from the environment. 

If you look at the glibc sources of strptime() you see what is done there: It just calls __strptime_internal. passing the current locale which includes the current TZ:


char * strptime (const char *buf, const char *format, struct tm *tm)
{
  return __strptime_internal (buf, format, tm, NULL, _NL_CURRENT_LOCALE);
}


So in glibc there is a function where UTC0 simply could be passed to, but unfortunately it is declared as hidden. In glibc's time.h here is the declaration:


/* strptime support.  */
extern char * __strptime_internal (const char *rp, const char *fmt,
                                   struct tm *tm, void *statep, locale_t locparam) attribute_hidden;


If we could call that function directly from datevec.m instead of wrapping strptime wtih setenv(TZ), everything would immediately be fine. Btw: A lot of software is facing such problems, e.g. the timestamps of files shown in the windows explorer change with daylight saving... 



Lars Kindermann <larskindermann>
Sat 12 Jan 2019 04:22:01 AM UTC, comment #22: 

Could someone point me at the articles you mention about strptime() interacting with TZ? I've been googling and can't find anything. The strptime() references I've found don't mention TZ at all.

POSIX: https://pubs.opengroup.org/onlinepubs/007904875/functions/strptime.html
glibc: http://man7.org/linux/man-pages/man3/strptime.3.html

Andrew Janke <apjanke>
Tue 15 May 2018 07:50:06 PM UTC, comment #21: 

Ok, so you are seeing the warning message in the command window:


warning: time stamp for '/.../dummy.m' is in the future
...


Yes I think that should be a separate bug report, and probably needs details of the type of your file system, whether it is a disk or a thumb drive or a network drive, and what the actual time stamp is reported as in the system file browser.

Mike Miller <mtmiller>
Group Member
Tue 15 May 2018 07:36:08 PM UTC, comment #20: 

Mike, You misread my post. I was not talking about the file being reported as modified but about the file being reported as having a timestamp in the future.

In fact my problem has nothing to do with the editor, although that is what I foolishly thought initially. It is to do with the interpreter monitoring changes to files in the octave path using services that are TZ-dependent.

As far as I can see it is a new problem and needs a new bug report. Happy to submit it if you agree.

Ian McCallion <ianmcc>
Tue 15 May 2018 04:15:58 PM UTC, comment #19: 

Ian - I think that the following script should show the popup message that the file dummy.m has been modified


system ('touch dummy.m);
pause (2);
dummy


without any kind of time zone manipulation. So I find it hard to separate that valid behavior from whatever influence changing the local time zone may have, to see whether there are still any problems on the development branch.

Mike Miller <mtmiller>
Group Member
Tue 15 May 2018 11:10:48 AM UTC, comment #18: 

I too have been having problems with using the TZ environment variable. The script below demonstrates one of the problems I found that may relate to this bug. It shows an unwanted interaction between using TZ for the purposes of the application and Octave's probably inappropriate dependency on the current value of of TZ in monitoring the path for changed files. (Caveat. I'm on windows)

Before launching this script, create a valid do-nothing function called dummy.m. An empty file does not show the problem

---------
% Before launching this script, create a valid do-nothing function called dummy.m. an empty file does not show the problem
system('touch dummy.m') % make sure timestamp is NOW
setenv('TZ', 'UTC0'); % !!!!! USE A TIMEZONE THAT IS WESTERLY FOR YOUR LOCATION !!!!
mktime(localtime(1)); % This is necessary only in Octave for Windows to bypass a bug in the posix layer which will be reported separately
pause (2); % Does not always happen without the pause

% the next command should report that the file has a timestamp in the future
dummy
setenv('TZ', '')
mktime(localtime(1));
-----------

Ian McCallion <ianmcc>
Mon 14 May 2018 07:16:36 PM UTC, comment #17: 

Confirmed, but this already happens in a version prior to the changeset. Could you please open a separate bug report?

Torsten Lilge <ttl>
Group Member
Mon 14 May 2018 05:41:44 AM UTC, comment #16: 

Update: the changeset introduced a new bug, now timestamps in the file browser are never updated any more.

Test: open a file in the editor, watch the time stamp and save.

Lars Kindermann <larskindermann>
Sun 13 May 2018 07:50:37 AM UTC, comment #15: 

This has solved the problem for me.

Lars Kindermann <larskindermann>
Sat 12 May 2018 05:16:06 PM UTC, comment #14: 

I have pushed changeset
http://hg.savannah.gnu.org/hgweb/octave/rev/c89fa0989e7b
which solves the issue when changing the timezone via setenv.

Torsten Lilge <ttl>
Group Member
Fri 20 Apr 2018 08:36:12 PM UTC, comment #13: 

Currently the QFileSystemWatcher is used for scanning open files for changes outside the editor. A solution might be to keep track of the file modification date in UTC and check this date when the fileChanged signal from the watcher is received.

Torsten Lilge <ttl>
Group Member
Fri 20 Apr 2018 07:37:31 PM UTC, comment #12: 

@Mike, if you plan to use any getenv(TZ) in the the gui, be aware that using setenv() makes also getenv() thread unsave.

The problem is not non-reentrant code but setenv() changes the pointer to the env-string which can cause getenv() to segfault.

Here is a good explanation, especially the part about TZ:
https://rachelbythebay.com/w/2017/01/30/env/

Lars Kindermann <larskindermann>
Fri 20 Apr 2018 06:47:36 PM UTC, comment #11: 

If there is a reentrant bug with calling setenv in a multi-threaded program, then we will work around that as needed.

As it is, setenv is a function that is provided to the interpreter for user functions to call, and I don't see that changing. And the Octave interpreter itself is single-threaded.

I see no problem with setting TZ to UTC0 temporarily while strptime is called and restoring it. If there is any problem, it is that the scope could be reduced a bit so it's only around the strptime function.

The bigger problem is that the GUI is reacting to this change, which is giving a wrong alert that files have been modified. That should be fixed regardless of whether the datevec function is changed.

Mike Miller <mtmiller>
Group Member
Fri 20 Apr 2018 06:30:55 PM UTC, comment #10: 

I think the culprit that really has to be avoided is

setenv ("TZ", "UTC0")

because "Modifications of environment variables are not allowed in multi-threaded programs". In glibc the setenv() function is MT-unsafe. See

https://www.gnu.org/software/libc/manual/html_node/Environment-Access.html#Environment-Access

But I'm aware why Mike has introduced the temporary switch to UTC timezone. strptime() is otherwise almost unusable. It is even recommended to do so in several articles on using time related functions. But serious problems arise in multi-threaded programs. Imagine what happens if several threads tamper uncoordinated with the TZ variable...

Maybe the only 100% safe and correct way is to write an own replacement of strptime, which does not care about timezones and daylight saving at all. Perhaps the code from glibc can be stripped down to the bare minimum.

Lars Kindermann <larskindermann>
Fri 20 Apr 2018 05:28:50 PM UTC, comment #9: 

Yes, it's related. See bug #49119 for the problem about the GUI editor opening files when running the test suite.

The new wrinkle is the time stamp / file reload message, since the tests in scripts/time set and reset the TZ environment variable.

It occurs to me this could be avoided if the time checks being done on whether a file is modified are made time-zone-aware, or done with time_t values. So maybe the code that checks whether a file has been modified needs to be reviewed.

Mike Miller <mtmiller>
Group Member
Fri 20 Apr 2018 03:58:52 PM UTC, comment #8: 

I don't know whether it is related, but if I start the GUI and execute _run_test_suite_, several files are opened in the editor and I eventually dialog windows pop up with warnings about files changed by other applications and asking whether I want to reload them.

John W. Eaton <jwe>
Group administrator
Sat 14 Apr 2018 02:00:42 PM UTC, comment #7: 

The shown columns in the file browser can be selected in a context menu after right mouse click into the header of the file list.

There is a preference for the editor in the settings dialog that allows to automatically reload changed editor files without prompting. Maybe this might be a workaround.

Torsten Lilge <ttl>
Group Member
Tue 10 Apr 2018 07:59:43 AM UTC, comment #6: 

This makes the gui almost unusable for dealing with large amounts of timestamped datapoints imported from text files. (What I mainly use Octave for)

As a temporary workaround, starting Octave with the command


TZ=UTC0 octave --gui


avoids the permanently appearing "file in editor changed" popup.

Lars Kindermann <larskindermann>
Tue 10 Apr 2018 06:50:54 AM UTC, comment #5: 

commenting out the line


setenv ("TZ", "UTC0");


in datevec.m completely gets rid of the messages about changed files in the editor and flipping timestamps.

But I have no idea how to use strptime() without this hack.

Perhaps creating a mutex for a strptime wrapper and the relevant gui code?

Lars Kindermann <larskindermann>
Tue 10 Apr 2018 06:50:30 AM UTC, comment #4: 

Thanks, I tried clicking on the gear and I tried the Preferences dialog, I couldn't find a place that configured which columns are shown in the file browser.

I thought it may have to do with the datevec code that changes the time zone (which I wrote). I can confirm this behavior by running only 'test datevec' repeatedly in the command window, with any script file open in the editor.

Confirmed, and I don't have any ideas on solutions, other than improving the GUI in a future version to not poll file modification times as aggressively, or while Octave functions are running.

Mike Miller <mtmiller>
Group Member
Tue 10 Apr 2018 06:17:14 AM UTC, comment #3: 

I have localized the probable source of the problem.

It is indeed caused by massive use of datenum() in my scripts. Here is the minimum code that triggers the error 100% reproducible when there is a large amount of big datafiles to be parsed:


for fil = dir("*.dat")'
  disp (fil.name)
  FID = fopen (fil.name, 'r');
  f = textscan (FID, '%s %*[^\n]');
  fclose (FID);
  ts = datenum (f{1}, 'yyyymmdd-HHMMSS')
endfor



There seems to be a race condition between the interpreter and the gui caused by code in datevec.m:



function [found, y, m, d, h, mi, s] = __date_str2vec__ (ds, p, f, rY, ry, fy, fm, fd, fh, fmi, fs)

  ## Local time zone is irrelevant, and potentially dangerous, when using
  ## strptime to simply convert a string into a broken down struct tm.
  ## Set and restore TZ so time is parsed exactly as-is. See bug #36954.
  TZ_orig = getenv ("TZ");
  unwind_protect
    setenv ("TZ", "UTC0");

...
 some code that uses strptime()
...

  unwind_protect_cleanup
    if (isempty (TZ_orig))
      unsetenv ("TZ");
    else
      setenv ("TZ", TZ_orig);
    endif
  end_unwind_protect



When the gui checks the timestamp of files, either of those open in the editor or those displayed in the filebrowser while the TZ environment variable is temporarily modified by that code, it assumes the files have changed and redraws the timestamps, which are exactly off by the local TZ then.

The unwind_protect assures that TZ is definitely reset but does not protect against concurrent code.  


@Mike: You can turn on the display of filetimes in the filebrowser widget by right-clicking the header line...

The zero size octave-workspace is not the cause, but for some reason it increased the probability of the ocurrence for me.

Lars Kindermann <larskindermann>
Mon 09 Apr 2018 10:59:30 PM UTC, comment #2: 

I think you are seeing a couple different effects here that may or may not be related. Let's try to separate them and investigate them individually.

The pop up message saying that the script has been modified is probably due to the script itself changing directories or somehow modifying the path while it is running. I observed the same when running the '__run_test_suite__' command from the editor:


>> edit __run_test_suite__
## switch to editor, hit F5, switch back to command window, wait
## pop up message appears, __run_test_suite__ has been changed, do I want to reload?


The second effect with timestamps - where are you seeing timestamps changing? The file browser doesn't show timestamps on files for me. What widget shows this or what message or behavior makes you think timestamps are changing?

Can you give an example of the type of script you are running that shows this behavior? Is your script doing anything with Octave's date/time functions?

Mike Miller <mtmiller>
Group Member
Mon 09 Apr 2018 10:38:00 PM UTC, comment #1: 

This effect is caused by an empty file (0 bytes) called "octave-workspace" in the current folder, which gets occasionally created during a crash.

To trigger it on linux do a


rm octave-workspace
touch octave-workspace


and run a script in the gui which does some file io for a while.

Lars Kindermann <larskindermann>
Sun 01 Apr 2018 02:41:59 AM UTC, original submission:  

While running scripts which create some system load for a while two things happen after last pull for current dev:

Frequently a message pops up, incorrectly stating that the file in the editor has been changed externally on disk.

The timestamps in the filebrowser start to change continuously and show wrong times for some files.

Both effects stop when returning to the command prompt.

Some of the timestamps flip exactly two hours forward and back, which is also my UTC offset. Others do show other wrong times...

System: Xubuntu 16.04, ext4 filesystem.

Lars Kindermann <larskindermann>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by apjanke (Posted a comment)
  • -email is unavailable- added by ianmcc (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by ttl (Posted a comment)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by larskindermann (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-05-13 ttl StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2018-05-12 ttl StatusConfirmed Ready For Test
    2018-04-11 mtmiller SummaryGUI file browser date stamps / editor script file has been modified while using datenum / datevec GUI file browser date stamps / editor script file appear modified when parsing date strings
    2018-04-10 mtmiller Item GroupOther Unexpected Error or Warning
        StatusNeed Info Confirmed
        SummaryTimestamps in filebrowser continuously change under heavy load GUI file browser date stamps / editor script file has been modified while using datenum / datevec
    2018-04-09 mtmiller StatusNone Need Info

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code