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: add nowait for asyncio disconnect #76

Merged

Conversation

hyeongguen-song
Copy link
Contributor

@hyeongguen-song hyeongguen-song commented Nov 20, 2022

please check this.

relative pr redis/redis-py#2356

without this, you can meet this

redis/asyncio/connection.py:767: TypeError
    async def send_packed_command(
        self, command: Union[bytes, str, Iterable[bytes]], check_health: bool = True
    ) -> None:
        if not self.is_connected:
            await self.connect()
        elif check_health:
            await self.check_health()
    
        try:
            if isinstance(command, str):
                command = command.encode()
            if isinstance(command, bytes):
                command = [command]
            if self.socket_timeout:
                await asyncio.wait_for(
                    self._send_packed_command(command), self.socket_timeout
                )
            else:
                self._writer.writelines(command)
                await self._writer.drain()
        except asyncio.TimeoutError:
            await self.disconnect(nowait=True)
            raise TimeoutError("Timeout writing to socket") from None
        except OSError as e:
            await self.disconnect(nowait=True)
            if len(e.args) == 1:
                err_no, errmsg = "UNKNOWN", e.args[0]
            else:
                err_no = e.args[0]
                errmsg = e.args[1]
            raise ConnectionError(
                f"Error {err_no} while writing to socket. {errmsg}."
            ) from e
        except Exception:
>           await self.disconnect(nowait=True)
E           TypeError: FakeConnection.disconnect() got an unexpected keyword argument 'nowait'

@cunla
Copy link
Owner

cunla commented Nov 20, 2022

Can you add a test for it?

@hyeongguen-song
Copy link
Contributor Author

@cunla
you can test it with above 4.4.0rc2

@cunla
Copy link
Owner

cunla commented Nov 20, 2022

Thx for writing this, but the test does not test the disconnect() method you changed..

@hyeongguen-song
Copy link
Contributor Author

I don't know what should I do.. should I have to test nowait of redis-py?
When you test with 4.4.0rc2 without this change and run test, you can see error.

@cunla
Copy link
Owner

cunla commented Nov 21, 2022

Hi, as you can see, tests fail for redis-py below 4.4.
Please update your implementation to **kwargs and add a test for using this parameter.

@cunla
Copy link
Owner

cunla commented Nov 21, 2022

It passes, the failure is due to a github action bad permissions which I will fix separately.

@cunla cunla merged commit 33c9b14 into cunla:master Nov 21, 2022
@cunla cunla added the bug Something isn't working label Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants