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

nailgun connect timeout error message fix #7437

Conversation

Projects
None yet
2 participants
@cosmicexplorer
Copy link
Contributor

commented Mar 25, 2019

Problem

We previously had an untested code path (NailgunExecutor._await_socket()) which constructed a NailgunClient.NailgunError upon a timeout when connecting to a started nailgun subprocess. The NailgunError constructor was changed a while ago to create its own error message, and instead accept arguments such as the pid and pgrp in its constructor, but this untested code path hadn't been updated to provide those arguments. This caused a strange error when this unlikely timeout occurred while compiling zinc in an osx laptop environment.

Solution

  • Raise a self.InitialNailgunConnectTimedOut and convert it into a self.Error in a try/except instead of raising a NailgunError.
  • Add testing for nailgun connection timeouts.
  • Create a separate option --nailgun-subprocess-startup-timeout to cover the time to wait for the nailgun subprocess to start up, using --nailgun-timeout-seconds to cover the time to wait after that for the nailgun subprocess to begin producing output.

Result

Another code path in our nailgun interactions is tested, and a bug in that code path is fixed.

cosmicexplorer added some commits Mar 25, 2019

@cosmicexplorer cosmicexplorer requested review from stuhood and illicitonion Mar 25, 2019

traceback=sys.exc_info()[2]
)
except (socket.error, NailgunProtocol.ProtocolError) as e:
raise self._make_nailgun_error(e)

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 26, 2019

Member

It looks like the method body could be inlined again to preserve the traceback?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 28, 2019

Author Contributor

Thanks! Doing that now.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 28, 2019

Author Contributor

(that seems like a particular nonsensical pythonism)

@cosmicexplorer cosmicexplorer merged commit 4a6fc72 into pantsbuild:master Mar 28, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.