ISSUE-189: EventBaseConfig::FEATURE_FDS not supported on Windows

[inri13666]

• Added exclusion for Windows platform

Fixes #189

Test results phpunit.txt

[ghost]

Before merging this, we should look into the IOCP feature of libevent. pecl-event added support for specifying flags yesterday (with release 2.5.0). Specifying the IOCP_DISPATCHER flag makes libevent look for IOCP-based implementations instead of the usual select()-based implementations. So before we have done the proper research, we shouldn't merge this until we are sure that IOCP is a closed path.

And I'd prefer to explicitely skip the ext-event loop instead of providing halfbaked support.

[inri13666]

I'm not sure that skip for ext-event loop will be a solution, because, with small changes from this PR, I have up and running 40k connections on my Windows platform.
Fully agree with you about the newer version of the extension, it should be investigated.
But there is no precompiled windows version for php 7.1 :(

[ghost]

You say this is a small change, but it has a huge impact. By removing the feature we effectively remove watching FDs and sockets, which may allow Windows to be supported. But the loop has an inconsistent behaviour and feature support between OSes. Instead libuv may be a better alternative for Windows. (Still hoping PECL will build the DLLs soon.)

[inri13666]

Hm, there is no way right now to check if it supported, I hope that 2.5.0 have this functionality and of course, then it should be used to determine available features. But right now this workaround for Windows platforms allows using the library with ext-event version lower than 2.5.0 :)

Related issue https://bitbucket.org/osmanov/pecl-event/issues/53/eventbaseconfig-feature_fds-not-supported

But Exception will be thrown only from version 2.4.3 and upper

[clue]

@inri13666 Thanks for looking into this and filing this PR!

@CharlotteDunois You seem to have a good understanding what's going on under the OS level and I'm not sure I follow the discussion, perhaps you can help clear this up? It's my understanding that prior to applying this change, the ExtEventLoop does not work on Windows at all, whereas after applying it, it at least allows stream/socket resources?

I agree that IOCP might be the way forward for Windows here, but does this changeset really do any harm? Windows uses a concept of "handles" for most common streams instead of "file descriptors" anyway?

[ghost]

@clue The problem is that libevent simply prefers select() based implementations, but you can change this to prefer IOCP-based implementations (on Windows). With select() you can only use sockets, simply because select() is only for sockets. File descriptors (handles) can not be used with select(), instead you have to use WaitForMultipleObjects (the win32 implementation of PHP's select uses that approach).

I/O Completion Ports however have a much more powerful way of waiting for notifications. IOCP not only allow file descriptors, they also allow (winsock) sockets (and beyond that win32 event objects), not just either handles or sockets.

The harm done by the changeset is simply portability. You can make libevent wait for activity on sockets and file descriptors on *nix, but on Windows this will fail. Simply because the event loop takes whatever it can get. Sure, if this is properly documented it's not that much of a big deal. However at the same time the event loop does not allow any customizations of libevent (there's no way to pass in a custom event base). IOCP would offer here the oppurtinity to give portability (at the cost of minimum ext-event v2.5.0, however that's a low cost to the value we would get).

Well, this is all theory. I haven't had the time to look greater into libevent's IOCP. So I can't really say if it works like that and if it's that easy or simple.

Honestly I'm fine with either way, since personally I'm not using libevent on Windows. However I see some certain value lost if we don't take the oppurtinity to properly check out IOCP.

[clue]

@CharlotteDunois Thanks for your in depth explanation!

Just to clarify, the way I read the ext-event changelog (https://pecl.php.net/package-changelog.php?package=event), it means that:

• ext-event in any version with or without the flag set on Linux/Mac can handle any file descriptor
• ext-event < 2.4.4 reports a warning with the flag set on Windows and does not work at all
• ext-event = 2.4.4 throws an exception with the flag set on Windows and does not work at all
• ext-event >= 2.5.0 supports this flag on Windows and will use IOCP under the hood?
• ext-event in any version without the flag set on Windows can handle at least sockets?

@inri13666 Thank you for working on this. The way I understand your last post means that you've tested with this ext-event < 2.4.4 without the flag and could successfully handle thousands of connections? This is interesting because Windows seems to have a default FD_SETSIZE of 64 (https://docs.microsoft.com/en-us/windows/desktop/api/winsock2/nf-winsock2-select).

[inri13666]

@clue, yes, https://gist.github.com/inri13666/1e8d4e60ede974b97f86ed64a1fdb0e1

event

Event support => enabled
Sockets support => enabled
Debug support => disabled
Extra functionality support including HTTP, DNS, and RPC => enabled
OpenSSL support => disabled
Thread safety support => enabled
Extension version => 2.4.2
libevent2 headers version => 2.0.22-stable

[ghost]

@inri13666 @clue
Just an update: Using PHP 7.2.10 ZTS with pecl-event 2.5.3 on Windows 10 x64 Pro 1803 it is not possible to use arbitrary file descriptor types, even with specifying IOCP flag.

Creating the EventBase fails with

event_base_new_with_config: no event mechanism available

I'd like to propose to accept this change (although the if check should be switched (!==) and the comment above it), but it must be documented that pecl-event on Windows is not capable of accepting arbitrary file descriptor types. Instead if an user requires this feature, libuv should be used.

[inri13666]

Hi @CharlotteDunois

• if fixed
• added Known Issues to README.md

[ghost]

The class is called EventConfig, not EventBaseConfig (that's only the alias in the event loop implementation).

[inri13666]

fixed

[WyriHaximus]

Can you fix that line in .gitignore and squash all the commits

[WyriHaximus]

Could you also squash the commits together into one atomic commit?

[inri13666]

@WyriHaximus feel free to use Squash and merge for your request
All commits from my PR will be combined into one commit in the base branch.

or use this https://patch-diff.githubusercontent.com/raw/reactphp/event-loop/pull/192.diff to push changes :) Cuz' for me matter current state more than author rights :)