tahoe-lafs/tahoe-lafs
tahoe-lafs
/
tahoe-lafs
2
0
Fork 0
Code Issues Pull Requests 23 Packages Projects Releases 1 Wiki Activity

Add http storage timeouts #1227

Merged
itamarst merged 21 commits from 3940-http-timeouts into master 2022-11-28 16:03:50 +00:00
Conversation 1 Commits 21 Files Changed 6 +196 -52
itamarst commented 2022-11-17 17:20:33 +00:00 (Migrated from github.com)
Copy Link

Fixes https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3940

Also this has some fixes for the intermittent dirty reactor timeouts, and workaround for Tor integration test issues that started failing when GitHub Actions switched ubuntu-latest to 22.04 (I have filed an issue: https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3943)

Fixes https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3940 Also this has some fixes for the intermittent dirty reactor timeouts, and workaround for Tor integration test issues that started failing when GitHub Actions switched `ubuntu-latest` to 22.04 (I have filed an issue: https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3943)
coveralls commented 2022-11-22 17:39:01 +00:00 (Migrated from github.com)
Copy Link

Coverage Status

Coverage remained the same at 94.699% when pulling aa80c9ef47 on 3940-http-timeouts into ac524a3077 on master.

[![Coverage Status](https://coveralls.io/builds/54620581/badge)](https://coveralls.io/builds/54620581) Coverage remained the same at 94.699% when pulling **aa80c9ef4748cf10e3b448b298df8b589c35cafd on 3940-http-timeouts** into **ac524a30770850587ece085e3ec252a27e76fd7b on master**.
exarkun (Migrated from github.com) approved these changes 2022-11-23 16:00:57 +00:00
exarkun (Migrated from github.com) left a comment
Copy Link

Thanks! Some comments inline. Please address and merge when you are satisfied.

Thanks! Some comments inline. Please address and merge when you are satisfied.
src/allmydata/storage/http_client.py
exarkun (Migrated from github.com) commented 2022-11-23 15:42:13 +00:00
Copy Link

Does the timeout DelayedCall leak if there is an error while collecting the response?

Does the timeout DelayedCall leak if there is an error while collecting the response?
src/allmydata/storage/http_client.py
@ -372,2 +373,4 @@
with CBOR) and set as the request body.
Default timeout is 60 seconds.
"""
exarkun (Migrated from github.com) commented 2022-11-23 15:43:42 +00:00
Copy Link

mypy considers int to be a subclass of float, so timeout: float = 60 is fine

mypy considers int to be a subclass of float, so `timeout: float = 60` is fine
src/allmydata/storage/http_client.py
exarkun (Migrated from github.com) commented 2022-11-23 15:50:20 +00:00
Copy Link

Maybe it should be clock instead of _clock

Maybe it should be `clock` instead of `_clock`
exarkun (Migrated from github.com) commented 2022-11-23 15:52:40 +00:00
Copy Link

Or StorageClient could offer a higher-level interface like self._client.decode_response(new_decode_cbor) that takes responsibility for managing the timeout and then the cbor (and other) decoders can remain ignorant of time.

Or `StorageClient` could offer a higher-level interface like `self._client.decode_response(new_decode_cbor)` that takes responsibility for managing the timeout and then the cbor (and other) decoders can remain ignorant of time.
exarkun (Migrated from github.com) commented 2022-11-23 15:55:34 +00:00
Copy Link

Or maybe StorageClientGeneral could just have a direct reference to a clock.

Or maybe `StorageClientGeneral` could just have a direct reference to a clock.
src/allmydata/test/common_system.py
exarkun (Migrated from github.com) commented 2022-11-23 15:57:25 +00:00
Copy Link

Maybe you can use reactor._internalReaders as a slightly more robust abstraction violation. Some of these classes might have their names changed in an upcoming Twisted release...

Maybe you can use `reactor._internalReaders` as a slightly more robust abstraction violation. Some of these classes might have their names changed in an upcoming Twisted release...
src/allmydata/test/common_system.py
exarkun (Migrated from github.com) commented 2022-11-23 15:58:18 +00:00
Copy Link

It's hard to love spin_until_cleanup_done but this new line is clearly less bad than the old line.

It's hard to love `spin_until_cleanup_done` but this new line is clearly less bad than the old line.
src/allmydata/test/test_storage_https.py
exarkun (Migrated from github.com) commented 2022-11-23 16:00:14 +00:00
Copy Link

I suppose AsyncTestCase.tearDown(self) might return a Deferred too.

I suppose `AsyncTestCase.tearDown(self)` might return a `Deferred` too.
itamarst (Migrated from github.com) reviewed 2022-11-23 16:28:23 +00:00
src/allmydata/test/test_storage_https.py
itamarst (Migrated from github.com) commented 2022-11-23 16:28:23 +00:00
Copy Link

At the moment it does not. But I could add a maybeDeferred I guess.

At the moment it does not. But I could add a maybeDeferred I guess.
itamarst (Migrated from github.com) reviewed 2022-11-28 15:03:13 +00:00
src/allmydata/storage/http_client.py
itamarst (Migrated from github.com) commented 2022-11-28 15:03:13 +00:00
Copy Link

Fixed.

Fixed.
itamarst (Migrated from github.com) reviewed 2022-11-28 15:12:44 +00:00
src/allmydata/storage/http_client.py
itamarst (Migrated from github.com) commented 2022-11-28 15:12:43 +00:00
Copy Link

I moved the CBOR decoding code into method of StorageClient.

I moved the CBOR decoding code into method of StorageClient.
Sign in to join this conversation.
Reviewers
No reviewers
exarkun
Labels
Clear labels   
Benchmarking and Performance

   
HTTP Storage Protocol

   
Nevow Removal

   
Python 3 Porting

   
not-for-merge
No Label
Benchmarking and Performance
HTTP Storage Protocol
Nevow Removal
Python 3 Porting
not-for-merge
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: tahoe-lafs/tahoe-lafs#1227
No description provided.
Delete Branch "3940-http-timeouts"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?