KSP-KOS / KOS

Fully programmable autopilot mod for KSP. Originally By Nivekk
Other
691 stars 229 forks source link

file open error results in crash. #2430

Closed Defaultldentity closed 3 years ago

Defaultldentity commented 5 years ago

When I do local f is open("0:/craft/survey.csv"). for a file that is absent, I get my script crashed and an error printed in the kOS terminal, contrary to the docs. Which behavior is correct? (I guess the docs probably are)

OPEN(PATH)

Will return a VolumeFile or VolumeDirectory representing the item pointed to by PATH.
It will return a Boolean false if there’s nothing present under the given path. Also see
Volume:OPEN.
Dunbaratu commented 5 years ago

Due to some conflicting views during development about how to handle things that can both return Boolean and not Boolean types from the same function, this didn't end up being implemented the way the docs had originally described it. *

I'd call this mostly a documentation error, and keep the behavior as is but fix the docs. The way you have to do it is to check if the file EXISTS() first before trying to OPEN() it.

* -> The original idea was that much like how in C you have IO calls that return a file handle OR a null if it can't open the file, and how a null is always Boolean false and any other pointer is true, so you could melt the meaning of "null" and "false" together and check if the returned handle is null by just pretending its Boolean. In the end this was not possible because we didn't want to implement NULL in kerboscript.

Defaultldentity commented 5 years ago

Some thoughts and observations.

  1. Volume:open() still returns false on error, like the docs say. I use this feature heavily.

  2. While testing if core:volume:open("test.txt") = false crashes, there is still a way to check for return type with :typename.

  3. So Volume:open() behavior is inconsistent with OPEN(). Same for other suffixes.

  4. AFAIK there is no (straightforward) way to atomically open-or-create a file, e.g. if multiple cores try to do i/o on archive. Testing for false helps with this task (we don't have exception handling yet?), leaving race condition possibility though. There is probably a solution with when magic, or MessageQueue though.

tl;dr please keep current Volume:open() behavior

Dunbaratu commented 3 years ago

closes for a duplicate of the other bug mentioned above.