 |
Smarty
WARNING: All discussion is moving to https://reddit.com/r/smarty, please go there! This forum will be closing soon. |
|
| View previous topic :: View next topic |
| Author |
Message |
boots
Administrator
Joined: 16 Apr 2003
Posts: 5611
Location: Toronto, Canada
|
 Posted: Mon Nov 06, 2006 8:51 pm Post subject: |
 |
|
Can you give some commentary? I'm not sure how that will help anything. In fact, it looks like it merely makes the windows issue worse by making it less performant (since it will eventually do two attempts at the rename).
If you tested this, how did you ensure that you were catching a race condition? I suspect that the simple fact that the timing is different for this version will result in the need for different test cases than you might be performing. In another thread I proposed a version that included a random time delay (and IIR, a retry). Is that the basis of this proposal -- introducing a time-lag?
Best. |
|
| Back to top |
|
c960657
Smarty Regular
Joined: 07 May 2003
Posts: 75
Location: Copenhagen, Denmark
|
| Posted: Mon Nov 06, 2006 10:23 pm Post subject: |
|
|
| boots wrote: | | Can you give some commentary? I'm not sure how that will help anything. In fact, it looks like it merely makes the windows issue worse by making it less performant (since it will eventually do two attempts at the rename). |
Yes, it is slightly less performant on Windows in the case where the compiled file already exists, i.e. when the template source has been changed.
If tests show that the performance hit is considerable (I doubt it, but I am not familiar with PHP on Windows), one could keep the old behaviour when the platform is Windows.
| boots wrote: | | If you tested this, how did you ensure that you were catching a race condition? |
The problem with the current code is that the compiled file ceases to exist for a split second (between unlink() and rename()). This will not be the case when the first rename() in my patch returns true. Two concurrent threads may compile the file at the same time, but the compiled files will be identical (assuming compiler functions etc. are deterministic), so it doesn't matter which thread "wins" - the compiled file will just be written twice, but it will exist without interruption.
I haven't been able to find an authorized reference that clearly states that rename() is atomic, but it is mentioned different places. E.g. this post by Rasmus Lerdorf describes a similar strategy.
Also I haven't been able to find a reference that explains what happens when a file opened with fopen() is overwritten while the file is being read. I have made a few tests that indicate that the original file is still around even the file is overwritten while it is being read (but if that is a problem, it is also a problem today).
I haven't run the patch on our production servers yet, because I wanted to hear your comments first. But if we agree on a solution here, I can apply it to our servers (Linux) and see if the race condition causes problems as often as before.
| boots wrote: | | I suspect that the simple fact that the timing is different for this version will result in the need for different test cases than you might be performing. In another thread I proposed a version that included a random time delay (and IIR, a retry). Is that the basis of this proposal -- introducing a time-lag? |
No, I haven't considered that. I'm not sure exactly what you are suggesting? |
|
| Back to top |
|
boots
Administrator
Joined: 16 Apr 2003
Posts: 5611
Location: Toronto, Canada
|
| Posted: Tue Nov 07, 2006 2:01 am Post subject: |
|
|
Actually, the code suggestion I proposed was in this thread (previous page) but now I remember that it only applies during the flock() cycle -- so just ignore that last comment.
Now, back to your patch: after another look-over of this issue, it does seem to me that it would be better to try and avoid the unlink as you are trying to do -- at least on filesystems that don't require it, meaning most *nix filesystems. The proposal from earlier in the thread then has some merit:
| Code: | if (PHP_OS == 'Windows' && file_exists($params['filename'])) {
@unlink($params['filename']);
} |
Unfortunately, it is overly specific and in the end, we really do have no other means than to actually do a test probe to determine if we should unlink or not -- just as your code does. Given all of that and the fact that we don't want to make things more expensive on Windows systems if we can avoid it, I would modify your code to add a short-circuit if statement (thereby only causing a single rename on Windows) to give a "best" (or maybe worst depending on how cynical you are) of both worlds:
| Code: | | if (PHP_OS == 'Windows' || !@rename($_tmp_file, $params['filename'])) { |
Of course, the order in the if() statement is important for the short-circuit to work on Windows systems. The caveat is that this still does not fix races but may mitigate unnecessary issues on (most) *nix systems. A nice side-benefit is that overall this should be less expensive than the current implementation on *nix systems while filesystems that can't rename in-place will still "work" whether they be Windows or not. In the latter case it will indeed be a little more expensive than the current implementation. In balance, this sounds like a good compromise to me.
I must say, I'm not keen on hearing that you intend to test on a production system. That said, I'll await some testing feedback from you and will also test myself on Windows. Assuming good feedback from the tests and also that we don't get any major objections here, I will commit these changes.
So what do you think? Anyone else?
ps. Thanks for taking the time to pick-up the ball and work on this subtle but annoying issue 
EDIT: So-far it seems to work okay on *nix and Win from my tests. |
|
| Back to top |
|
messju
Administrator
Joined: 16 Apr 2003
Posts: 3336
Location: Oldenburg, Germany
|
| Posted: Wed Nov 08, 2006 4:36 pm Post subject: |
|
|
I think a simple solution like the one proposed earler is fully acceptible.
(I mean this one:
| Code: | if (!@rename($_tmp_file, $params['filename'])) {
// Delete the file if it allready existed (this is needed on Win,
// because it cannot overwrite files with rename()
@unlink($params['filename']);
@rename($_tmp_file, $params['filename']);
}
@chmod($params['filename'], $smarty->_file_perms);
|
)
- we fix the issue on all filesystems that support an atomic overwriting rename().
- the problem that the performance may get worse on windows is neglible, IMHO: we are in core_write_file() so we already had a recompilation or a cache miss, these are both not the usual case and a few additional ms due to a failing rename() are realy acceptible.
just my 2ct |
|
| Back to top |
|
boots
Administrator
Joined: 16 Apr 2003
Posts: 5611
Location: Toronto, Canada
|
| Posted: Wed Nov 08, 2006 5:28 pm Post subject: |
|
|
| messju wrote: | | - the problem that the performance may get worse on windows is neglible, IMHO: we are in core_write_file() so we already had a recompilation or a cache miss, these are both not the usual case and a few additional ms due to a failing rename() are realy acceptible. |
As amazing as it may sound, I don't agree with you here -- not when we can so easily avoid the extra call. Then again, for some customers I'm forced to host some internal sites on Windows servers so I have a vested interest in trying to minimize time .
I want to note that my real issue here is with cache misses (or more aptly, rebuilding caches) rather than recompiles. I have sites that rebuild MANY page content images every 60 seconds (based on constantly changing data feeds). If I can avoid a few hundred failed renames per minute, that is a good thing in my books.
I won't nit-pick it, but I don't really see the benefit in not adding in the short-circuit.
best. |
|
| Back to top |
|
messju
Administrator
Joined: 16 Apr 2003
Posts: 3336
Location: Oldenburg, Germany
|
| Posted: Wed Nov 08, 2006 5:50 pm Post subject: |
|
|
| boots wrote: | | I won't nit-pick it, but I don't really see the benefit in not adding in the short-circuit. |
Yes. Maybe I was too biased, since I am not affected by the mutilated rename() on certain platforms.
The shortcut is a good thing (tm?). |
|
| Back to top |
|
boots
Administrator
Joined: 16 Apr 2003
Posts: 5611
Location: Toronto, Canada
|
| Posted: Wed Nov 08, 2006 6:15 pm Post subject: |
|
|
| messju wrote: | | The shortcut is a good thing (tm?). |
That's short-circuit -- not shortcut  lol
I agree that it is a real PITA to have to support clients that insist on Windows -- but what can be done? Someone has to pay the bills  |
|
| Back to top |
|
boots
Administrator
Joined: 16 Apr 2003
Posts: 5611
Location: Toronto, Canada
|
| Posted: Wed Nov 08, 2006 7:01 pm Post subject: |
|
|
| This is now committed to CVS. Anyone who can, please test. |
|
| Back to top |
|
boots
Administrator
Joined: 16 Apr 2003
Posts: 5611
Location: Toronto, Canada
|
| Posted: Mon Nov 20, 2006 8:53 pm Post subject: |
|
|
Can anyone add any testing feedback here? I normally don't continually ask for feedback but considering that we are touching a relatively core piece of code, I'm just trying to ensure enough real-world coverage.
FWIW, I'm satisfied with the results and can foresee no particular issues. |
|
| Back to top |
|
c960657
Smarty Regular
Joined: 07 May 2003
Posts: 75
Location: Copenhagen, Denmark
|
| Posted: Mon Nov 20, 2006 9:02 pm Post subject: |
|
|
| boots wrote: | | Can anyone add any testing feedback here? |
I have been using this on production servers since 8 November. So far no problems here. |
|
| Back to top |
|
|
|
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
|
Powered by phpBB © 2001, 2005 phpBB Group
|
|
