Skip to content

Conversation

@enumag
Copy link
Contributor

@enumag enumag commented Apr 20, 2020

@enumag
Copy link
Contributor Author

enumag commented Apr 20, 2020

Honestly I don't understand why Travis didn't catch this...

@kelunik
Copy link
Member

kelunik commented Apr 20, 2020

Because Travis uses glibc instead of musl. I think we should conditionally use that flag then.

@enumag
Copy link
Contributor Author

enumag commented Apr 20, 2020

Oh so the constant still does exist with glibc? Should I add a ternary operator to use it if it exists? I have no idea what it's for...

@trowski
Copy link
Member

trowski commented Apr 20, 2020

As I understood, the flag requires writes to be successfully written to disk before returning (synchronous). In context of this library, since the writing is done in a separate thread, we can know the data was actually written to disk when the promise succeeds.

Since we aren't able to use the flag in other drivers (or apparently some platforms), I think let's just remove it.

@kelunik
Copy link
Member

kelunik commented Apr 20, 2020

In context of this library, since the writing is done in a separate thread, we can know the data was actually written to disk when the promise succeeds.

No, we can't know that without FSYNC, we just know that the OS wrote the data to its internal buffers, but in case of a power failure the data might still be lost, see https://linux.die.net/man/2/fsync.

Please change it to a conditional, yes.

@kelunik
Copy link
Member

kelunik commented Apr 20, 2020

@enumag See rosmanov/pecl-eio@65e8b54#diff-0cc087e71ac0c866a224fb4846b3314dR932

@trowski
Copy link
Member

trowski commented Apr 20, 2020

No, we can't know that without FSYNC, we just know that the OS wrote the data to its internal buffers, but in case of a power failure the data might still be lost, see https://linux.die.net/man/2/fsync.

Reading that, it seems like what I said is true. Am I not understanding something?

@kelunik
Copy link
Member

kelunik commented Apr 20, 2020

@trowski I understood you said we don't need the flag, because we do writes in another thread?

@trowski
Copy link
Member

trowski commented Apr 20, 2020

No, I phrased it poorly I guess. My point was that we use the flag for synchronous writes to ensure the data is written, but the synchronous write happens in another thread, so it's non-blocking.

However, only EIO exposes this flag, so behavior is inconsistent between drivers and apparently between platforms. I suggested removing the flag for consistency. I'm also fine with using it conditionally if we don't care about that.

@kelunik
Copy link
Member

kelunik commented Apr 20, 2020

We could add fflush to BlockingDriver as well. Should we?

@kelunik kelunik changed the title Remove usage of undefined constant EIO_O_FSYNC Skip usage of EIO_O_FSYNC if undefined Apr 21, 2020
@kelunik kelunik merged commit 7dc65ff into amphp:master Apr 21, 2020
@kelunik
Copy link
Member

kelunik commented Apr 21, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants