New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non-persistent and edge-triggered modes for FileDescriptorEvent. #1596

Merged
merged 2 commits into from Dec 5, 2016

Conversation

Projects
None yet
2 participants
@japplegame
Contributor

japplegame commented Oct 15, 2016

Both non-persistent and edge-triggered modes work perfectly for my libpq (PostgreSQL) wrapper.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 19, 2016

Member

If edge-triggers works, I'd prefer to remove the non-persistent option for now, to reduce the implementation burden for other drivers and the new vibe-core module. Otherwise, looks good, except that the libasync and win32 drivers also need to be adjusted to take the additional parameter (even though they only define a stub implementation).

Member

s-ludwig commented Oct 19, 2016

If edge-triggers works, I'd prefer to remove the non-persistent option for now, to reduce the implementation burden for other drivers and the new vibe-core module. Otherwise, looks good, except that the libasync and win32 drivers also need to be adjusted to take the additional parameter (even though they only define a stub implementation).

@japplegame

This comment has been minimized.

Show comment
Hide comment
@japplegame

japplegame Oct 20, 2016

Contributor

I'm not sure that the edge-triggered events are supported by all backends. What about platforms without epoll?
In addition, I encountered problems with my libpq wrapper. In edge-triggered mode sometimes waiting for read event is endless. Because libpq reading function (PQconsumeInput) most likely reads not all available data from the socket. To avoid endless waiting you can use ioctl to check data availability or use non-persistent mode.

Contributor

japplegame commented Oct 20, 2016

I'm not sure that the edge-triggered events are supported by all backends. What about platforms without epoll?
In addition, I encountered problems with my libpq wrapper. In edge-triggered mode sometimes waiting for read event is endless. Because libpq reading function (PQconsumeInput) most likely reads not all available data from the socket. To avoid endless waiting you can use ioctl to check data availability or use non-persistent mode.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 20, 2016

Member

In the libpq wrapper, do you always try to read before waiting for an event? In that case it should work regardless of the read function reading everything available.

Edge-triggered can be simulated internally with the non-persistent approach without causing issues, but the reverse is not possible. For that reason, I'd like to go with the most restricted mode, rather than facilitating dependent code to rely on the more relaxed semantics of non-persistent.

Member

s-ludwig commented Oct 20, 2016

In the libpq wrapper, do you always try to read before waiting for an event? In that case it should work regardless of the read function reading everything available.

Edge-triggered can be simulated internally with the non-persistent approach without causing issues, but the reverse is not possible. For that reason, I'd like to go with the most restricted mode, rather than facilitating dependent code to rely on the more relaxed semantics of non-persistent.

@japplegame

This comment has been minimized.

Show comment
Hide comment
@japplegame

japplegame Oct 20, 2016

Contributor

In the libpq wrapper, do you always try to read before waiting for an event? In that case it should work regardless of the read function reading everything available.

Just trying to read isn't enough, because next edge-triggered event will be send only after reading function returns EAGAIN. But unfortunately I have no way to control the reading process as it is hidden inside libpq library. All I have is a function for reading portion of data from a socket. This function returns only information about errors, but nothing about data availability.

Edge-triggered can be simulated internally with the non-persistent approach without causing issues, but the reverse is not possible. For that reason, I'd like to go with the most restricted mode, rather than facilitating dependent code to rely on the more relaxed semantics of non-persistent.

Agree.

Contributor

japplegame commented Oct 20, 2016

In the libpq wrapper, do you always try to read before waiting for an event? In that case it should work regardless of the read function reading everything available.

Just trying to read isn't enough, because next edge-triggered event will be send only after reading function returns EAGAIN. But unfortunately I have no way to control the reading process as it is hidden inside libpq library. All I have is a function for reading portion of data from a socket. This function returns only information about errors, but nothing about data availability.

Edge-triggered can be simulated internally with the non-persistent approach without causing issues, but the reverse is not possible. For that reason, I'd like to go with the most restricted mode, rather than facilitating dependent code to rely on the more relaxed semantics of non-persistent.

Agree.

@japplegame

This comment has been minimized.

Show comment
Hide comment
@japplegame

japplegame Nov 29, 2016

Contributor

libasync and win32 drivers are adjusted.
Unfortunately I can't avoid nonPersistent mode in my libpg wrapper.
I suggest not to remove this mode.

Contributor

japplegame commented Nov 29, 2016

libasync and win32 drivers are adjusted.
Unfortunately I can't avoid nonPersistent mode in my libpg wrapper.
I suggest not to remove this mode.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Nov 29, 2016

Member

Okay, let's keep it for now. I'm going to deal with it somehow for the new backend (which could mean . Would be interesting anyway to see if libpq can somehow be fixed/wrapped to avoid it.

Do you see any value in the persistent mode? AFAICS removing it wouldn't break existing use and it often tends to result in 100% CPU load anyway.

Member

s-ludwig commented Nov 29, 2016

Okay, let's keep it for now. I'm going to deal with it somehow for the new backend (which could mean . Would be interesting anyway to see if libpq can somehow be fixed/wrapped to avoid it.

Do you see any value in the persistent mode? AFAICS removing it wouldn't break existing use and it often tends to result in 100% CPU load anyway.

@japplegame

This comment has been minimized.

Show comment
Hide comment
@japplegame

japplegame Nov 29, 2016

Contributor

Use of edge-triggered mode instead of non-persistent sometimes leads to strange infinite waiting for read events. At now I can't understand the reasons, but will try.

Contributor

japplegame commented Nov 29, 2016

Use of edge-triggered mode instead of non-persistent sometimes leads to strange infinite waiting for read events. At now I can't understand the reasons, but will try.

@japplegame

This comment has been minimized.

Show comment
Hide comment
@japplegame

japplegame Dec 4, 2016

Contributor

Finally, I managed to get to work my libpq wrapper with edge-triggered events.

Contributor

japplegame commented Dec 4, 2016

Finally, I managed to get to work my libpq wrapper with edge-triggered events.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Dec 5, 2016

Member

Cool! That means at least that there is less pressure on the vibe-core implementation in that regard.

I'll merge the PR like it is an will then gradually deprecate the level triggered mode. I'll also still see if I can make the one-shot mode work for vibe-core.

Member

s-ludwig commented Dec 5, 2016

Cool! That means at least that there is less pressure on the vibe-core implementation in that regard.

I'll merge the PR like it is an will then gradually deprecate the level triggered mode. I'll also still see if I can make the one-shot mode work for vibe-core.

@s-ludwig s-ludwig merged commit 64551cf into vibe-d:master Dec 5, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

s-ludwig added a commit that referenced this pull request Dec 19, 2016

Merge pull request #1596 from japplegame/fde_mode
Non-persistent and edge-triggered modes for FileDescriptorEvent.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment