Added an option to specify a timeout duration

[sanders41]

Closes #231

I did not implement this with the Session as suggested in the issue simply because in the requests documentation there doesn't seem to be a way to set a default timeout for the session. It is possible with httpx to do this so maybe I'm missing something in requests. If anyone knows how and would rather use a session I can update it.

As an added bonus this does speed up the tests significantly be setting a timeout of 1 second on the communication error test.

[curquiza]

Hello @sanders41!
@bidoubiwa will be a good reviewer for your PR. She has a lot on her plate right now, but she will take a look as soon as possible 🙂

[bidoubiwa]

Hey @sanders41. Thanks so much for this addition as it is something we might consider on all repositories. In order to create a base example for other SKD's to follow, i have some requests to further improve your PR.

We made a custom Error called MeiliSearchTimeoutError is it possible to add it on timeOut?

Would it be possible to add some more tests?

For example:

• 0 timeout on the correct host should fail (contrary to the test you have added on a bad host)
• Custom timeout should work
• No custom timeout should work
• Inside the Index instance, is the timeout present, if yes, could you compare the values with your inputted timeout.

[sanders41]

@bidoubiwa here is what I have done so far, let me know what you think.

• I changed the timeout error to a MeiliSearchTimeoutError as requested.
• Added a test with setting and not setting a custom timeout.
• I have tried to come up with a way to do the 0 timeout error and I'm having issues here. requests won't let you set a timeout of of 0. I was able to get this test to work locally by setting the timeout to 1e-10 (effectively so low it's almost 0), but the reason the tests are failing CI now is because even the 1e-10 isn't fast enough here to cause the timeout error. Thoughts on what to do about this?
• In catching the timeout exception I needed to remove the timeout from the test_meilisearch_communication_error_host test in order for a connection error to raise instead of a timeout error, which puts this test back to being long running. For me this test causes the test suite to stall for around 5 minutes before the connection error is returned. Then running tox to test all python versions, the test suite takes about 20 minutes to complete because it stalls here with each version of Python. With this in mind I patched the requests.post on this test to make it raise the connection exception right away rather than hanging. Take a look and see if you are OK with what I did here.

Inside the Index instance, is the timeout present, if yes, could you compare the values with your inputted timeout.

This one I'm not exactly sure what you mean. I'll give a little more info here on how it works, then from that maybe you can give a little more info on what test you would want for this?

When the client is instantiated the timeout value is passed to the Config initializer. So when the client is created the timeout values is persistent across every request as long as this client is used.

[sanders41]

I have tried to come up with a way to do the 0 timeout error and I'm having issues here. requests won't let you set a timeout of of 0. I was able to get this test to work locally by setting the timeout to 1e-10 (effectively so low it's almost 0), but the reason the tests are failing CI now is because even the 1e-10 isn't fast enough here to cause the timeout error. Thoughts on what to do about this?

I tried something new with this and it passes locally, but fails CI. My only thought right now is a difference in the way Mac and Linux are handling the error. In CI it is skipping the timeout error and falling trough to a connection error. When I can get a chance I will test this theory on one of my Linux computers.

[bidoubiwa]

Thanks so much for this throughout and really interesting research! About the tests with timeout 0, we can let it go until we find a good way to test it! It is not the most important one :)

This one I'm not exactly sure what you mean. I'll give a little more info here on how it works, then from that maybe you can give a little more info on what test you would want for this?

This one is really a basic test to ensure the right timeout is set.

client = meilisearch.Client("http://wrongurl:1234", MASTER_KEY, timeout=1)
assert client.config.timeout == 1

client = meilisearch.Client("http://wrongurl:1234", MASTER_KEY)
assert client.config.timeout == 10

If this is possible of course

[sanders41]

I did some testing on both Mac and Linux and came up with a way to get the tests to work on both. Basically the issue came down to Linux was just responding so fast it didn't time out. I figured a way to slow the test down to give it a chance to time out on Linux also.

[bidoubiwa]

Based on this doc

[bidoubiwa]

🔥 Great, great addition! Sorry for the delay 😅

[bidoubiwa]

bors merge

[bors]

Build succeeded:

• integration-tests (3.6)
• integration-tests (3.7)
• integration-tests (3.8)
• integration-tests (3.9)
• pylint