Edgewall Software
Modify

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#11329 closed defect (fixed)

Configuration file may not be reparsed when successive save operations are called on a low resolution filesystem

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.0.2
Component: general Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:

On platforms with a low resolution timestamp, each save operation will modify the file timestamp so that the file is sure to be reparsed.

API Changes:

The function wait_for_file_mtime_change can be placed before a save operation to ensure that the timestamp will be changed by the call to save.

Description (last modified by Ryan J Ollos)

As discussed in comment:17:ticket:11069 and comment:13:ticket:11176, the configuration file might not be reparsed when successive save operations are invoked in a time interval less that is less than the resolution of the filesystem.

In #11069 we discussed how to fix the issue for the functional test environment. Changes will be proposed here that intend to fix the issue for a Trac instance running on a low resolution filesystem.

Attachments (0)

Change History (9)

comment:1 by Ryan J Ollos, 6 years ago

Proposed changes can be found in log:rjollos.git:t11329.

After putting the test cases in place, I encountered failures on EXT4 that could only be avoided by putting a sleep operation of up to 1e-2 between touch / save operations in the test case.

Waiting for the file modification time to change seems like the most general solution that should work on all platforms, regardless of the file timestamp resolution and how small an interval there is between touch or save operations.

comment:2 by Remy Blank, 6 years ago

This solution could lead to multiple environment reloads when saving a configuration, under normal (non-test) operation.

Instead of trying to work around the low timestamp resolution of certain filesystems (and the per-platform differences), how about storing the last change time in a separate file (trac.ini.ts)?

comment:3 by Ryan J Ollos, 6 years ago

Description: modified (diff)

in reply to:  2 ; comment:4 by Remy Blank, 6 years ago

Replying to rblank:

This solution could lead to multiple environment reloads when saving a configuration, under normal (non-test) operation.

Ah, scratch that, I misunderstood something. But storing the timestamp as a file instead of using the mtime would still have the benefit of avoiding up to 1 second of delay in tests, and therefore avoid making them (even) slower.

in reply to:  4 ; comment:5 by Ryan J Ollos, 6 years ago

Replying to rblank:

Ah, scratch that, I misunderstood something.

After you mentioned I was considering that this might be possible - in the while loop we wait for the file modification time to change in response to the equivalent of a touch -m command, and then save the file. If an option value was accessed in another thread in the time between the touch -m and the save, then I think the file would be needlessly reparsed. That time interval must be quite small however.

I might not be thinking about that correctly though since I still haven't sorted out the path by which parse_if_needed gets called when an option value is accessed, assuming that I'm even correct in assuming it works that way.

But storing the timestamp as a file instead of using the mtime would still have the benefit of avoiding up to 1 second of delay in tests, and therefore avoid making them (even) slower.

There are two relevant factors:

  • the 1 second delay will only be seen on filesystems with a low resolution timestamp.
  • the 1 second delay will only be seen when there are repeated save operations that would not each result in a change to the modification time of the configuration file.

Given those two factors, I'm not sure we'll ever see too much of a slowdown in the test execution time, and at least with the current set of tests we'll only see a few seconds slowdown. Either way we should certainly go for the most robust solution.

I'll investigate the timestamp in file solution. If I understand correctly, we could really just store anything that was unique and changed each time the config file changed, such as the fingerprint of the file, or a random hash, but the timestamp is simple and probably makes the most sense. Would there be any advantage to storing it in a database table (system?) rather than a file?

in reply to:  5 comment:6 by Remy Blank, 6 years ago

Replying to rjollos:

Given those two factors, I'm not sure we'll ever see too much of a slowdown in the test execution time, and at least with the current set of tests we'll only see a few seconds slowdown.

Your explanation makes sense, yes. There's one easy way to find out: run the tests on an affected system, with and without the patch. If the difference is indeed only a few seconds, we shouldn't implement anything more complicated.

comment:7 by Ryan J Ollos, 6 years ago

Status: newassigned

comment:8 by Ryan J Ollos, 6 years ago

API Changes: modified (diff)
Release Notes: modified (diff)

Here are the times for an OS with a low resolution timestamp filesystem (EXT3), based on n = 3 executions of each:

  • Before: 203-211 seconds (with 13 test failures in the authorization tests, like the one shown in comment:17:ticket:11069)
  • After: 253-257 seconds

On an OS with a high resolution timestamp filesystem (EXT4), the execution time is insignificantly affected by the change, and the execution time is 220-225 seconds. So I think the slowdown is probably closer to 25 seconds since the Before tests on EXT3 would take longer to execute if the failing tests ran to completion.

The slowdown is greater than I expected, but since most developers will work on a modern filesystem, I wouldn't expect it to affect development efforts much. Let me know if anyone thinks the solution will be problematic and I can investigate alternatives.

With the change in place, we should be able to update these hints:

since the configuration file should always be reparsed after a save operation, even on a platform with a low resolution timestamp. Regardless, it is probably better advice to use a platform with a high resolution timestamp.

I added an improvement to the functional tests, based on a hint from Jun in comment:54:ticket:5658: [31658dca/rjollos.git]. The full set of changes can be found in log:rjollos.git:t11329.2.

Last edited 6 years ago by Ryan J Ollos (previous) (diff)

comment:9 by Ryan J Ollos, 6 years ago

Resolution: fixed
Status: assignedclosed

Committed to 1.0-stable in [12258] and merged to trunk in [12259].

Documentation updates:

Last edited 6 years ago by Ryan J Ollos (previous) (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.