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

fix master runner not close rpc server #1935

Merged
merged 4 commits into from
Dec 20, 2021
Merged

Conversation

lizhaode
Copy link
Contributor

No description provided.

@cyberw
Copy link
Collaborator

cyberw commented Nov 22, 2021

That test was added by @delulu about 2 years ago. Maybe he/she can explain it?

@lizhaode
Copy link
Contributor Author

@cyberw
I don't know why in test_test_stop_event unit test, master.quit() will raise error like this

Traceback (most recent call last):
  File "/home/runner/work/locust/locust/locust/test/test_runners.py", line 909, in test_test_stop_event
    master.quit()
  File "/home/runner/work/locust/locust/locust/runners.py", line 825, in quit
    self.server.send_to_client(Message("quit", None, client.id))
  File "/home/runner/work/locust/locust/locust/util/exception_handler.py", line 13, in wrapper
    return function(*args, **kwargs)
  File "/home/runner/work/locust/locust/locust/rpc/zmqrpc.py", line 29, in send_to_client
    raise RPCError("ZMQ sent failure") from e
locust.exception.RPCError: ZMQ sent failure

could kindly help me, thanks

@cyberw
Copy link
Collaborator

cyberw commented Nov 23, 2021

Sorry, I dont have time atm. Lets hope @delulu responds.

@cyberw
Copy link
Collaborator

cyberw commented Dec 16, 2021

I'm going to close this due to inactivity soon. Sorry there hasnt been any clarification on what that test does. Do you have a specific issue you are trying to fix with this change?

@lizhaode
Copy link
Contributor Author

I'm going to close this due to inactivity soon. Sorry there hasnt been any clarification on what that test does. Do you have a specific issue you are trying to fix with this change?

the rpc server will not close when use locust as a library, so I add self.server.close() in quit def

@cyberw
Copy link
Collaborator

cyberw commented Dec 17, 2021

I see. You can change delulus test case from

master.quit()

to something like

self.assertRaises(RpcError, master.quit) # mocked rpc runner throws an exception on quit

Or maybe change the mock to not throw an exception makes more sense...

@lizhaode
Copy link
Contributor Author

Thanks for help.

But I try to understand this test case.

That found in this test case:

  1. it start a master runner
  2. start a worker runner
  3. assert something
  4. master send quit event
  5. call master.quit() func

The error is occurred by self.server.send_to_client func. Maybe in step 2, master and worker runner start a real rpc server and client?

So, it should not raise error?

@cyberw
Copy link
Collaborator

cyberw commented Dec 17, 2021

Aha, I thought it was due to the special code in MockedRpcServerClient ( https://github.com/locustio/locust/blob/master/locust/test/test_runners.py#L89), but I guess that's not where the error is coming from.

Then I dont have any ideas, sorry.

@lizhaode
Copy link
Contributor Author

Aha, I thought it was due to the special code in MockedRpcServerClient ( https://github.com/locustio/locust/blob/master/locust/test/test_runners.py#L89), but I guess that's not where the error is coming from.

Then I dont have any ideas, sorry.

I found the reason maybe.

1st call master_env.events.quitting.fire

then call master.quit(), the client is closed already, so the quit func can not send rpc message

@cyberw
Copy link
Collaborator

cyberw commented Dec 20, 2021

thx!

@cyberw cyberw merged commit cce0d29 into locustio:master Dec 20, 2021
cyberw added a commit that referenced this pull request Jan 29, 2022
Fix "socket operation on non-socket" at shutdown, by reverting #1935
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants