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

Create _misc_loop() task thread safely. Fixes #39 #40

Merged

Conversation

flyte
Copy link
Collaborator

@flyte flyte commented Feb 24, 2021

Fixes #39

@frederikaalund
Copy link
Collaborator

Good catch. :) Simple and straight-forward fix as well. 👍

I think that your explanation in #39 is really good:

It's because the self._client.connect() call is done in an executor (because it blocks on socket creation).

In fact, it's so good that I'd like it to be part of the code. ;) This way, we let future maintainers know why we use a thread-safe variant in this specific case.

Add a reference to the run_in_executor part at line 79:

await loop.run_in_executor(None, self._client.connect, self._hostname, self._port, 60)  # [3]

Then reference this in a comment near your fix at line 303:

def _on_socket_open(self, client, userdata, sock):
    def cb():
        client.loop_read()
    self._loop.add_reader(sock.fileno(), cb)
    def create_task_cb():
        self._misc_task = self._loop.create_task(self._misc_loop())
    # paho-mqtt calls this function from another thread since we use an executor
    # for the `self._client.connect` call (see [3]). Therefore, we use a thread-safe
    # function to start the `_misc_loop` task.
    self._loop.call_soon_threadsafe(create_task_cb)

@flyte
Copy link
Collaborator Author

flyte commented Feb 24, 2021

Comments added. I somehow wrote a bunch of stuff before realising you'd given me comments to use, so they're a little different. Hope it still makes sense.

@frederikaalund
Copy link
Collaborator

Thanks. Looks good to me. 👍

@frederikaalund frederikaalund merged commit 23c3c82 into empicano:master Feb 24, 2021
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.

Exception in on_socket_open: Non-thread-safe operation invoked on an event loop other than the current one
2 participants