api: Use requests.Session #18

dehnert · 2017-07-19T07:41:18Z

Using requests.Session allows the requests library to reuse HTTP connections,
which is potentially helpful for performance. This fixes python-zulip-api #3.

Some notes:

I'm currently lazily initializing the Session object. This has the advantage if somebody constructs a client and then modifies some field before making any requests, it should work as they probably expect. It has the disadvantage of probably not being threadsafe (although the client as a whole might not be either), not to mention whatever style views people have.
It appears to pass lint. AFAIK, that's it for "testing" the API -- I think the bot tests mock it out? (But those tests do pass.)
I ran mypy (though not the version that Zulip has pinned). Beyond some warnings about stubs that I think are preexisting, I get some errors about request.Session's field typing. I think typeshed is just wrong; Stubs for requests are too restrictive python/typeshed#1093 certainly does not leave me with much faith in the stubs. Comparing Session's stubs to .request's stubs the former is certainly more restrictive, and I don't believe the stubs' typing for verify (which really does seem to take a string) or auth (which does seem to take an object -- though in fairness it looks like passing a (user,pass) tuple should work too, and the stubs support it). I'm not sure what to do about this, especially given that the API seems annotated but not currently being tested.

I think this is sane to merge, although you might reasonably want me to change how I handled these notes (or have other objections).

timabbott · 2017-07-20T23:04:16Z

zulip/zulip/__init__.py

@@ -327,6 +351,8 @@ def do_api_query(self, orig_request, url, method="POST", longpolling=False, file
        for f in files:
            req_files.append((f.name, f))

+        self._init_session()


maybe self.ensure_session() would be a better name here?

timabbott · 2017-07-20T23:07:54Z

The code looks good; posted one quick comment. Some thoughts on your notes:

I think threadsafety is something I'd rather handle via locking/whatever if we find a use for it.
For typeshed, I'd open an issue in Typeshed and use # type: ignore # link to issue to silence the warnings in our code and reference it so it's clear why and when we can clean it up. We'll be type-checking the API again eventually.
Correct, this has no real test suite in this project yet (there's a bit more of one in the server project in the form of tools/test-api there). Might be worth running that against this project, because I think there's a good chance that its mocking strategy won't work with this.

dehnert · 2017-07-23T04:27:16Z

Thanks for the feedback. I'm traveling this weekend, but I'll try to deal early next week.

…

On July 20, 2017 4:07:54 PM PDT, Tim Abbott ***@***.***> wrote: The code looks good; posted one quick comment. Some thoughts on your notes: * I think threadsafety is something I'd rather handle via locking/whatever if we find a use for it. * For typeshed, I'd open an issue in Typeshed and use `# type: ignore # link to issue` to silence the warnings in our code and reference it so it's clear why and when we can clean it up. We'll be type-checking the API again eventually. * Correct, this has no real test suite in this project yet (there's a bit more of one in the server project in the form of `tools/test-api` there). Might be worth running that against this project, because I think there's a good chance that its mocking strategy won't work with this. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #18 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

timabbott · 2017-07-25T20:33:46Z

Cool

timabbott · 2017-07-27T17:44:10Z

@dehnert quick ping on this :)

Using requests.Session allows the requests library to reuse HTTP connections, which is potentially helpful for performance. This fixes python-zulip-api #3.

dehnert · 2017-07-28T18:09:58Z

I fixed these comments last night, except that I haven't been able to run test-api yet -- my Zulip proper environment (without these changes) is failing to run the tests. On the typeshed front, I submitted python/typeshed#1504 upstream and referenced it from the code.

timabbott · 2017-07-28T18:16:42Z

@eeshangarg do you have time to quickly run test-api against an updated version python-zulip-api that contains this code?

rht · 2017-08-12T11:27:32Z

Passed in Travis ! (I
suppose a fresh dev build doesn't have the hysteresis of accumulated errs from
past experimentation.)

Tried porting a subset of the backend test in zulip/zulip
rht@2f96745
just enough to run ./tools/test-api but to no avail since the
./tools/provision from the zulip/zulip repo would still fetch the old version
instead of the hash being tested. So I treaded the path of least loc-patch by
patching the requirements in zulip/zulip.

timabbott · 2017-08-14T18:26:26Z

@rht for the zulip/zulip issue, you just need to update requirements.txt to point to the Git URL of this branch in the relevant fork, and then you can provision against this version.

timabbott · 2017-08-24T17:41:35Z

Merged, thanks @dehnert! I think it'll be easier to test by hand once it's merged and we can just include it in the server repo. It seems pretty unlikely that it'll fail in a nontrivial way.

dehnert · 2017-08-26T16:39:09Z

Thanks folks!

As of commit 5eaac7b (zulip#18), zulip.Client is not thread-safe and especially not fork-safe due to connections held open by requests.Session. Delay construction of the Client until after forking off zulip_to_zephyr. Replace the fork for each message sent by zephyr_to_zulip with a threaded queue worker. Signed-off-by: Anders Kaseorg <[email protected]>

As of commit 5eaac7b (#18), zulip.Client is not thread-safe and especially not fork-safe due to connections held open by requests.Session. Delay construction of the Client until after forking off zulip_to_zephyr. Replace the fork for each message sent by zephyr_to_zulip with a threaded queue worker. Signed-off-by: Anders Kaseorg <[email protected]>

zulipbot added the needs review label Jul 19, 2017

timabbott reviewed Jul 20, 2017

View reviewed changes

zulipbot added reviewed and removed needs review labels Jul 20, 2017

api: Use requests.Session

8ff2a14

Using requests.Session allows the requests library to reuse HTTP connections, which is potentially helpful for performance. This fixes python-zulip-api #3.

dehnert force-pushed the session branch from d405ca1 to 8ff2a14 Compare July 28, 2017 08:23

zulipbot added needs review and removed reviewed labels Jul 28, 2017

timabbott closed this Aug 24, 2017

zulipbot removed the needs review label Aug 24, 2017

This was referenced Sep 17, 2017

[wip!] Add slack data importer. #114

Closed

Add test-backend from Zulip server. #123

Open

dehnert deleted the session branch November 27, 2021 09:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: Use requests.Session #18

api: Use requests.Session #18

dehnert commented Jul 19, 2017

timabbott Jul 20, 2017

timabbott commented Jul 20, 2017

dehnert commented Jul 23, 2017 via email

timabbott commented Jul 25, 2017

timabbott commented Jul 27, 2017

dehnert commented Jul 28, 2017

timabbott commented Jul 28, 2017

rht commented Aug 12, 2017

timabbott commented Aug 14, 2017

timabbott commented Aug 24, 2017

dehnert commented Aug 26, 2017

api: Use requests.Session #18

api: Use requests.Session #18

Conversation

dehnert commented Jul 19, 2017

timabbott Jul 20, 2017

Choose a reason for hiding this comment

timabbott commented Jul 20, 2017

dehnert commented Jul 23, 2017 via email

timabbott commented Jul 25, 2017

timabbott commented Jul 27, 2017

dehnert commented Jul 28, 2017

timabbott commented Jul 28, 2017

rht commented Aug 12, 2017

timabbott commented Aug 14, 2017

timabbott commented Aug 24, 2017

dehnert commented Aug 26, 2017