GeezerVison

Do we need Py_fclose as well?

Do we need Py_fclose as well?

fclose seems to be enough, we don't need to wrap it(Unless we use CloseHandle to close the file).

@zooba:

Do we need Py_fclose as well?

Py_fopen() adds values to fopen(): accept a Python object, make the file descriptor non-inheritable, raise an exception on error.

Py_fclose() would just call fclose() without any additional value? I would prefer to not add it unless we have to, and call directly fclose(). I asked the reporter if Py_fclose() would be needed.

To be honest, I don't understand well the Windows DLL issue. From what I understood, to use a FILE* in Python DLL, it should be open in the Python DLL. Otherwise, things can go wrong if the C library version is not exactly the same in two DLLs. But I don't know if it's safe to call fopen() / Py_fopen() in DLL A and call fclose() in DLL B.

PyRun_AnyFileExFlags has "closeit" parameter which I always used.
But you are correct. Py_fopen() needs Py_fclose().

Thank you guys for doing all this.

But you are correct. Py_fopen() needs Py_fclose().

Is Py_fclose() needed because of the DLL issue?

Is Py_fclose() needed because of the DLL issue?

Yes. When you call Py_fopen(), you will get a FILE * from whichever C runtime Python is linked to, which isn't necessarily the same as your own application. So you can only pass it back to the same C runtime - it must only be passed into Python APIs, essentially. So regular fclose might crash (or close a different file), and you need Py_fclose() to make sure it goes back to the right runtime.

Ok, I wanted to make sure that I get the rationale correctly.

I added Py_fclose() function and documented that files opened by Py_fopen() must only be closed by Py_fclose().

Your updated documentation seems a bit snarky. The Py_fclose may be needed on any platform depending on your static linking situation.

I see no reason not to just document it as "should be used to close files opened by Py_fopen" and if people manage to use fclose and it works then good for them.

Your updated documentation seems a bit snarky. The Py_fclose may be needed on any platform depending on your static linking situation.

It's not my intent to be snarky. I just tried to explain why calling fclose() directly can cause issues in practice.

I see no reason not to just document it as "should be used to close files opened by Py_fopen" and if people manage to use fclose and it works then good for them.

Ok, let's do that.

I created capi-workgroup/decisions#51: "Add public Py_fopen() and Py_fclose() functions, and remove private _Py_fopen_obj() function".

@serhiy-storchaka: I tried to implement all requested tests, but I skipped some of them for practical reasons.

On Windows, the file mode is limited to 10 characters. If it's a problem later, I can allocate memory on the heap and write more complicated code to handle longer modes. In practice, we should be fine with this arbitrary limit.

PR merged, thanks for your reviews!

@serhiy-storchaka: You can write a follow-up PR if you want to extend the code coverage (add more tests).