Skip to content
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

Ignored pywatchman.SocketTimeout in Watchman autoreloader. #11303

Merged
merged 1 commit into from May 4, 2019

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Apr 29, 2019

These are expected with the non-blocking loop.

Only with other errors, e.g. WatchmanError('empty watchman response')
after killing watchman need to check the server status.

@felixxm
Copy link
Member

felixxm commented Apr 30, 2019

@orf Can you take a look?

@orf
Copy link
Sponsor Contributor

orf commented Apr 30, 2019

I’m confused @blueyed, can you elaborate some more on why this is needed? I’m really not sure about skipping socket timeouts here, surely they are indicative of an issue?

@blueyed
Copy link
Contributor Author

blueyed commented Apr 30, 2019

It is not needed (i.e. does not fix a bug), but just avoids the version request every 5 seconds.

For reference: https://github.com/facebook/watchman/blob/0513bb4dc6719b49d1ee6455ac538457bc64491c/python/pywatchman/__init__.py#L271-L278

I'd even argue that we should not have a timeout here in the first place, i.e. just let it block there until there is either a response or the socket gets closed (e.g. when watchman gets killed), but thought to rather play it safe.

@blueyed
Copy link
Contributor Author

blueyed commented Apr 30, 2019

With this it is also (easier) possible to add debug logging here for unexpected things: #11305.

@blueyed
Copy link
Contributor Author

blueyed commented Apr 30, 2019

It requires pywatchman v3.8.0 (facebook/watchman@1e9d5cb), 4 years old.

@orf
Copy link
Sponsor Contributor

orf commented Apr 30, 2019

Looks good to me, thanks for the explanation 👍

@felixxm
Copy link
Member

felixxm commented May 1, 2019

I checked and SocketTimeout had appeared in pywatchman 1.2.0. Can you add a note to the ref/django-admin.txt about minimum supported version of pywatchman? Thanks.

@felixxm felixxm self-assigned this May 1, 2019
Bumped minimum supported pywatchman version to 1.2.0.

These exceptions don't require checking a server status.
@felixxm
Copy link
Member

felixxm commented May 3, 2019

I rebased from master and added a note about bumping supported pywatchman version to 1.2.0.

@felixxm felixxm changed the title Improved watchman autoreloader to pass on SocketTimeouts. Ignored pywatchman.SocketTimeout in Watchman autoreloader. May 3, 2019
@felixxm felixxm merged commit 29601bc into django:master May 4, 2019
@blueyed blueyed deleted the watchman branch May 5, 2019 07:36
@blueyed
Copy link
Contributor Author

blueyed commented May 5, 2019

Thanks, what about #11305 then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants