Fix OSError: [WinError 87] The parameter is incorrect #1454

euri10 · 2022-04-20T13:05:20Z

That PR fixes the iocp error raised when workers > 1 by forcing the loop in windows the same way we do when reload is used.
I think that's the right fix, cant think of another option since we default to the iocp loop and it seems to clash with multiple processes.

Kludex · 2022-04-27T05:47:48Z

uvicorn/loops/asyncio.py

+        if reload:
+            logger.warning(
+                "The --reload flag should not be used in production on Windows."
+            )


I'm probably missing something. But why do we have this warning here? 🤔 Why is it assuming this place is "windows production"?

I don't think I'm missing anything... I think this warning is just wrong.

I finally understand why this warning is here! It's because of this comment: #1220 (comment)

It has the wrong assumption, tho! It's not because we reach this code that it's running in production.

Kludex · 2022-05-05T06:36:33Z

@euri10 Can we pass a flag use_subprocess instead of both reload and workers? Or... Even better, can we check on asyncio_setup() function if we are running on a subprocess? I guess that's the case we want to set the SelectorEventLoop.

smitchell6879 · 2022-06-07T12:25:32Z

So I am guessing this was never fixed?

Kludex · 2022-06-07T13:08:14Z

Yep. That's correct.

@euri10 is away for some time, so anyone can take this PR, and help. Steps:

try to understand my comments
see if they are reasonable
reply my comments
change the implementation if needed
I'll review it again
merge and release

Comments about the staleness are unnecessary. Someone motivated about this issue needs to take ownership over it.

I can help on all the steps. :)

euri10 · 2022-06-13T06:44:50Z

@euri10 Can we pass a flag use_subprocess instead of both reload and workers? Or... Even better, can we check on asyncio_setup() function if we are running on a subprocess? I guess that's the case we want to set the SelectorEventLoop.

I added a property for that, I'm not sure how to check if we are in a subprocess at running time

smitchell6879 · 2022-06-13T07:27:38Z

Didn't mean to offend about the efforts of this pull request. I was working on it and had a solution that wasn't liked. So I assumed those that ended my pull request had a better idea of what they wanted and I just wanted to know if there was progress as for now I am running my version.

Kludex · 2022-06-18T15:52:22Z

uvicorn/loops/asyncio.py

-def asyncio_setup(reload: bool = False) -> None:  # pragma: no cover
-    if sys.version_info >= (3, 8) and sys.platform == "win32" and reload:
+def asyncio_setup(use_subprocess: bool = False) -> None:  # pragma: no cover
+    if sys.version_info >= (3, 8) and sys.platform == "win32" and use_subprocess:
        logger.warning("The --reload flag should not be used in production on Windows.")


This warning is wrong now, because we can reach this line with the --workers.

yep indeed, so the use_subprocess flag is cool on the surface but does not allow to discriminate between workers usage and reload usage so I think I'm gonna revert to having both in the signature despite your comment here (#1454 (comment)), what you think ? or you get an idea as to detect at running time the fact we're in a subprocess ?

The thing is that the warning itself doesn't make sense to me. If uvicorn is running under --reload and we show the warning, it's not needed, because it's not in production anyway... How do we even know that is being used in production?

ok let's dig why we put this in the first place, cant remember

it was introduced in #1257

If we just want to add a warning message because we want to be sure users don't use reload in production, that should be in the main.py, not on the loop setup.

ok so let's keep use_subprocess and remove the warning ?

oh and f*** windows

ok so let's keep use_subprocess and remove the warning ?

Yes 👍

If we still want the warning in the future, we can add it before those lines:

uvicorn/uvicorn/main.py

Lines 552 to 557 in b1a4582

if config.should_reload:

sock = config.bind_socket()

ChangeReload(config, target=server.run, sockets=[sock]).run()

elif config.workers > 1:

sock = config.bind_socket()

Multiprocess(config, target=server.run, sockets=[sock]).run()

I think the warning is acceptable, as we want to be sure the user doesn't make the mistake of running --reload in production. But that's for another PR/discussion.

should be good for another round of review @Kludex

This reverts commit d1b5f83

Kludex · 2022-06-21T18:34:48Z

tests/test_config.py

+def test_config_use_subprocess(reload, workers, expected):
+    config = Config(app=asgi_app, reload=reload, workers=workers)
+    config.load()
+    assert config.use_subprocess == expected


use_process doesn't exist anymore on this branch. I was fine with it tho 🤔

ok I reverted too much, goinmg to do that tomorrow sorry :_)

mixed up things etc... tired

No problem! Have a good evening man! ♥️🙏

ok should be ok now @Kludex , apologies for the confusion :)

Kludex

🙏

* Fix OSError: [WinError 87] The parameter is incorrect * Added use_subprocess to be more generic * Black * Revert "Added use_subprocess to be more generic" This reverts commit d1b5f83 * Removed reload warning * Min diff, not sure where this came from * Min diff 2 * Redo use_subprocess

euri10 mentioned this pull request Apr 20, 2022

Fix OSError occurs when workers > 1 #1317 #1443

Closed

Kludex reviewed Apr 27, 2022

View reviewed changes

Kludex added the windows label May 5, 2022

Kludex added the waiting author Waiting for author's reply label Jun 12, 2022

Kludex requested changes Jun 18, 2022

View reviewed changes

Kludex added this to the Version 0.18.0 milestone Jun 18, 2022

Kludex mentioned this pull request Jun 18, 2022

Version 0.18.0 #1524

Merged

5 tasks

euri10 added 5 commits June 21, 2022 18:20

Fix OSError: [WinError 87] The parameter is incorrect

0219897

Added use_subprocess to be more generic

5329864

Black

0d36ebe

Revert "Added use_subprocess to be more generic"

b0ac2ed

This reverts commit d1b5f83

Removed reload warning

15877c2

euri10 force-pushed the win87error branch from 708c5f3 to 15877c2 Compare June 21, 2022 16:20

euri10 added 2 commits June 21, 2022 18:22

Min diff, not sure where this came from

7c03ef5

Min diff 2

a13821b

Kludex reviewed Jun 21, 2022

View reviewed changes

Redo use_subprocess

baec732

Kludex approved these changes Jun 22, 2022

View reviewed changes

Kludex merged commit 99ead36 into encode:master Jun 22, 2022

euri10 deleted the win87error branch June 22, 2022 08:05

Kludex mentioned this pull request Jul 25, 2022

Run uvicorn with multiple workers on Windows - WinError 87 #342

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix OSError: [WinError 87] The parameter is incorrect #1454

Fix OSError: [WinError 87] The parameter is incorrect #1454

euri10 commented Apr 20, 2022

Kludex Apr 27, 2022

Kludex May 5, 2022

Kludex May 5, 2022

Kludex commented May 5, 2022

smitchell6879 commented Jun 7, 2022

Kludex commented Jun 7, 2022 •

edited

Loading

euri10 commented Jun 13, 2022

smitchell6879 commented Jun 13, 2022

Kludex Jun 18, 2022

euri10 Jun 20, 2022

Kludex Jun 20, 2022

euri10 Jun 20, 2022

euri10 Jun 20, 2022

Kludex Jun 20, 2022

euri10 Jun 20, 2022

euri10 Jun 20, 2022

Kludex Jun 20, 2022

euri10 Jun 21, 2022

Kludex Jun 21, 2022

euri10 Jun 21, 2022

euri10 Jun 21, 2022

Kludex Jun 21, 2022

euri10 Jun 22, 2022

Kludex left a comment

	if config.should_reload:
	sock = config.bind_socket()
	ChangeReload(config, target=server.run, sockets=[sock]).run()
	elif config.workers > 1:
	sock = config.bind_socket()
	Multiprocess(config, target=server.run, sockets=[sock]).run()

Fix OSError: [WinError 87] The parameter is incorrect #1454

Fix OSError: [WinError 87] The parameter is incorrect #1454

Conversation

euri10 commented Apr 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kludex commented May 5, 2022

smitchell6879 commented Jun 7, 2022

Kludex commented Jun 7, 2022 • edited Loading

euri10 commented Jun 13, 2022

smitchell6879 commented Jun 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kludex left a comment

Choose a reason for hiding this comment

Kludex commented Jun 7, 2022 •

edited

Loading