I happened upon Monty’s suggested changes to file opening. They look pretty reasonable to me but since I don’t use Windows and it looks mostly like changes that relate to the old Windows issue of running out of file descriptors and needing to GC, I really can’t have a meaningful opinion. Are we going to incorporate the changes?
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: RIW: Re-Invent Wheel
Hi Tim,
Main request was to avoid case of race condition that can happen when separating the action (like opening a file) from the query in case of failure (like was file unwritable?), because another concurrent program might have changed the file system status in between (think of concurrent squeak images creating concurrent /tmp/foobar.baz for example).
I have already integrated the necessary VM changes, but it remains to integrate image side changes. Last time I checked there were unecessary changes mixed that I'd preferred to see discussed here, and the fallback code might not work in older VM, then I came short of spare time... But in any cases, there are too many .mcz roting in inbox, so I'm glad another pair of eyes analyze the changes.
2017-08-28 20:26 GMT+02:00 tim Rowledge tim@rowledge.org:
I happened upon Monty’s suggested changes to file opening. They look pretty reasonable to me but since I don’t use Windows and it looks mostly like changes that relate to the old Windows issue of running out of file descriptors and needing to GC, I really can’t have a meaningful opinion. Are we going to incorporate the changes?
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: RIW: Re-Invent Wheel
On 28-08-2017, at 11:47 AM, Nicolas Cellier nicolas.cellier.aka.nice@gmail.com wrote: Main request was to avoid case of race condition that can happen when separating the action (like opening a file) from the query in case of failure (like was file unwritable?), because another concurrent program might have changed the file system status in between (think of concurrent squeak images creating concurrent /tmp/foobar.baz for example).
Oh, *that* problem. I hate that. It’s such a stupid bit of (mis)design. I think I’ve been complaining about that for around 30 years…
I have already integrated the necessary VM changes, but it remains to integrate image side changes. Last time I checked there were unecessary changes mixed that I'd preferred to see discussed here, and the fallback code might not work in older VM, then I came short of spare time... But in any cases, there are too many .mcz roting in inbox, so I'm glad another pair of eyes analyze the changes.
It’s all very well Torvalds declaring that all bugs are shallow with enough pairs of eyes, *if* you have enough pairs of eyes. We need more.
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: RSC: Rewind System Clock
Not only my eyes are old, my brain too: here is the original intention of Monty:
https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/140/commits/c1db1eb40...
So it avoids file existence test in pre-condition, not in post-condition... I swear I had seen other changes about passing failure status back to image for avoiding post-condition tests, but maybe it was not in this batch of changes.
Forget about all I wrote before, the changes definitely deserve a proper review. Maybe check what happens in case someone did not update the VM properly before applying the image changes...
2017-08-29 1:36 GMT+02:00 tim Rowledge tim@rowledge.org:
On 28-08-2017, at 11:47 AM, Nicolas Cellier <nicolas.cellier.aka.nice@
gmail.com> wrote:
Main request was to avoid case of race condition that can happen when
separating the action (like opening a file) from the query in case of failure (like was file unwritable?), because another concurrent program might have changed the file system status in between (think of concurrent squeak images creating concurrent /tmp/foobar.baz for example).
Oh, *that* problem. I hate that. It’s such a stupid bit of (mis)design. I think I’ve been complaining about that for around 30 years…
I have already integrated the necessary VM changes, but it remains to
integrate image side changes.
Last time I checked there were unecessary changes mixed that I'd
preferred to see discussed here, and the fallback code might not work in older VM, then I came short of spare time...
But in any cases, there are too many .mcz roting in inbox, so I'm glad
another pair of eyes analyze the changes.
It’s all very well Torvalds declaring that all bugs are shallow with enough pairs of eyes, *if* you have enough pairs of eyes. We need more.
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: RSC: Rewind System Clock
2017-08-29 8:25 GMT+02:00 Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com>:
Not only my eyes are old, my brain too: here is the original intention of Monty:
https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/140/commits/ c1db1eb40b73373b4e4fe6541cda74c8c742edc7
So it avoids file existence test in pre-condition, not in post-condition... I swear I had seen other changes about passing failure status back to image for avoiding post-condition tests, but maybe it was not in this batch of changes.
Ah, it seems I'm senile since I can't remember it's me who integrated those changes, but not completely senile since at least I remember that I saw such changes ;)
-------------------------------
Name: VMMaker.oscog-nice.2233 Author: nice Time: 5 June 2017, 6:13:56.418116 pm UUID: ff8286e4-b96f-4ce6-9cb5-e7b286679e03 Ancestors: VMMaker.oscog-eem.2232
Integrate changes from monty to sqFileOpenNew.
There must be a way to check if failure is due to existence of file (implemented via PrimErrInappropriate). If we would test existence afterward, the file could have been deleted in interim leading to another race condition.
This VMMaker change requires a coordinated platform source change (adding a 4th parameter to sqFileOpenNew).
Forget about all I wrote before, the changes definitely deserve a proper review. Maybe check what happens in case someone did not update the VM properly before applying the image changes...
2017-08-29 1:36 GMT+02:00 tim Rowledge tim@rowledge.org:
On 28-08-2017, at 11:47 AM, Nicolas Cellier <
nicolas.cellier.aka.nice@gmail.com> wrote:
Main request was to avoid case of race condition that can happen when
separating the action (like opening a file) from the query in case of failure (like was file unwritable?), because another concurrent program might have changed the file system status in between (think of concurrent squeak images creating concurrent /tmp/foobar.baz for example).
Oh, *that* problem. I hate that. It’s such a stupid bit of (mis)design. I think I’ve been complaining about that for around 30 years…
I have already integrated the necessary VM changes, but it remains to
integrate image side changes.
Last time I checked there were unecessary changes mixed that I'd
preferred to see discussed here, and the fallback code might not work in older VM, then I came short of spare time...
But in any cases, there are too many .mcz roting in inbox, so I'm glad
another pair of eyes analyze the changes.
It’s all very well Torvalds declaring that all bugs are shallow with enough pairs of eyes, *if* you have enough pairs of eyes. We need more.
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: RSC: Rewind System Clock
The relevant packages are:
Files-monty.172.mcz Multilingual-monty.226.mcz 51Deprecated-monty.48.mcz.
The last two are refactorings: instead of overriding #open:forWrite:, the post initialization code goes in separate #opened:forWrite: methods, which the non-overridden superclass #open:forWrite: now sends.
#retryWithGC:until:forFileNamed: is an ugly hack, the second argument is always used the same way, and it's sent from methods that don't even depend on file descriptor availability. I just refactored it to take two arguments and to not be sent from methods that can't benefit from it.
I would like to rewrite #primOpenNew: to backwards-compatibly simulate the primitive using the broken, race condition Squeak code for older VMs. I tried this originally by checking for PrimErrNotFound on failure, but that weirdly only works the first time, and in subsequent invocations the error code is nil. Not sure if it's a bug. However, I could do the same thing even better using something like "Smalltalk vm version".
So let me first upload a new Files.mcz with this backwards compatible behavior.
Sent: Wednesday, August 30, 2017 at 4:13 PM From: "Nicolas Cellier" nicolas.cellier.aka.nice@gmail.com To: "The general-purpose Squeak developers list" squeak-dev@lists.squeakfoundation.org Subject: Re: [squeak-dev] Inbox/Files-monty.172
2017-08-29 8:25 GMT+02:00 Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com[mailto:nicolas.cellier.aka.nice@gmail.com]>:
Not only my eyes are old, my brain too: here is the original intention of Monty:
https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/140/commits/c1db1eb40...] So it avoids file existence test in pre-condition, not in post-condition... I swear I had seen other changes about passing failure status back to image for avoiding post-condition tests, but maybe it was not in this batch of changes.
Ah, it seems I'm senile since I can't remember it's me who integrated those changes, but not completely senile since at least I remember that I saw such changes ;)
Name: VMMaker.oscog-nice.2233 Author: nice Time: 5 June 2017, 6:13:56.418116 pm UUID: ff8286e4-b96f-4ce6-9cb5-e7b286679e03 Ancestors: VMMaker.oscog-eem.2232
Integrate changes from monty to sqFileOpenNew.
There must be a way to check if failure is due to existence of file (implemented via PrimErrInappropriate). If we would test existence afterward, the file could have been deleted in interim leading to another race condition.
This VMMaker change requires a coordinated platform source change (adding a 4th parameter to sqFileOpenNew).
Forget about all I wrote before, the changes definitely deserve a proper review.Maybe check what happens in case someone did not update the VM properly before applying the image changes...
2017-08-29 1:36 GMT+02:00 tim Rowledge <tim@rowledge.org[mailto:tim@rowledge.org]>:
On 28-08-2017, at 11:47 AM, Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com[mailto:nicolas.cellier.aka.nice@gmail.com]> wrote: Main request was to avoid case of race condition that can happen when separating the action (like opening a file) from the query in case of failure (like was file unwritable?), because another concurrent program might have changed the file system status in between (think of concurrent squeak images creating concurrent /tmp/foobar.baz for example).
Oh, *that* problem. I hate that. It’s such a stupid bit of (mis)design. I think I’ve been complaining about that for around 30 years…
I have already integrated the necessary VM changes, but it remains to integrate image side changes. Last time I checked there were unecessary changes mixed that I'd preferred to see discussed here, and the fallback code might not work in older VM, then I came short of spare time... But in any cases, there are too many .mcz roting in inbox, so I'm glad another pair of eyes analyze the changes.
It’s all very well Torvalds declaring that all bugs are shallow with enough pairs of eyes, *if* you have enough pairs of eyes. We need more.
tim
tim Rowledge; tim@rowledge.org[mailto:tim@rowledge.org]; http://www.rowledge.org/tim%5Bhttp://www.rowledge.org/tim] Strange OpCodes: RSC: Rewind System Clock
Hi, there.
Note that new deprecations in Trunk should go into the "60-Deprecated" package at the moment.
Best, Marcel Am 06.09.2017 04:36:59 schrieb monty monty2@programmer.net: The relevant packages are:
Files-monty.172.mcz Multilingual-monty.226.mcz 51Deprecated-monty.48.mcz.
The last two are refactorings: instead of overriding #open:forWrite:, the post initialization code goes in separate #opened:forWrite: methods, which the non-overridden superclass #open:forWrite: now sends.
#retryWithGC:until:forFileNamed: is an ugly hack, the second argument is always used the same way, and it's sent from methods that don't even depend on file descriptor availability. I just refactored it to take two arguments and to not be sent from methods that can't benefit from it.
I would like to rewrite #primOpenNew: to backwards-compatibly simulate the primitive using the broken, race condition Squeak code for older VMs. I tried this originally by checking for PrimErrNotFound on failure, but that weirdly only works the first time, and in subsequent invocations the error code is nil. Not sure if it's a bug. However, I could do the same thing even better using something like "Smalltalk vm version".
So let me first upload a new Files.mcz with this backwards compatible behavior.
Sent: Wednesday, August 30, 2017 at 4:13 PM From: "Nicolas Cellier" To: "The general-purpose Squeak developers list" Subject: Re: [squeak-dev] Inbox/Files-monty.172
2017-08-29 8:25 GMT+02:00 Nicolas Cellier :
Not only my eyes are old, my brain too: here is the original intention of Monty:
https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/140/commits/c1db1eb40...] So it avoids file existence test in pre-condition, not in post-condition... I swear I had seen other changes about passing failure status back to image for avoiding post-condition tests, but maybe it was not in this batch of changes.
Ah, it seems I'm senile since I can't remember it's me who integrated those changes, but not completely senile since at least I remember that I saw such changes ;)
Name: VMMaker.oscog-nice.2233 Author: nice Time: 5 June 2017, 6:13:56.418116 pm UUID: ff8286e4-b96f-4ce6-9cb5-e7b286679e03 Ancestors: VMMaker.oscog-eem.2232
Integrate changes from monty to sqFileOpenNew.
There must be a way to check if failure is due to existence of file (implemented via PrimErrInappropriate). If we would test existence afterward, the file could have been deleted in interim leading to another race condition.
This VMMaker change requires a coordinated platform source change (adding a 4th parameter to sqFileOpenNew).
Forget about all I wrote before, the changes definitely deserve a proper review.Maybe check what happens in case someone did not update the VM properly before applying the image changes...
2017-08-29 1:36 GMT+02:00 tim Rowledge :
On 28-08-2017, at 11:47 AM, Nicolas Cellier wrote: Main request was to avoid case of race condition that can happen when separating the action (like opening a file) from the query in case of failure (like was file unwritable?), because another concurrent program might have changed the file system status in between (think of concurrent squeak images creating concurrent /tmp/foobar.baz for example).
Oh, *that* problem. I hate that. It’s such a stupid bit of (mis)design. I think I’ve been complaining about that for around 30 years…
I have already integrated the necessary VM changes, but it remains to integrate image side changes. Last time I checked there were unecessary changes mixed that I'd preferred to see discussed here, and the fallback code might not work in older VM, then I came short of spare time... But in any cases, there are too many .mcz roting in inbox, so I'm glad another pair of eyes analyze the changes.
It’s all very well Torvalds declaring that all bugs are shallow with enough pairs of eyes, *if* you have enough pairs of eyes. We need more.
tim
tim Rowledge; tim@rowledge.org[mailto:tim@rowledge.org]; http://www.rowledge.org/tim%5Bhttp://www.rowledge.org/tim] Strange OpCodes: RSC: Rewind System Clock
squeak-dev@lists.squeakfoundation.org
-
Marcel Taeumel -
monty -
Nicolas Cellier -
tim Rowledge