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.

Sign up for GitHub

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

Jump to bottom

Excessive handshakes in desktop client #6053

Closed
6 of 11 tasks
charlag opened this issue Oct 30, 2023 · 0 comments · Fixed by #6072
Closed
6 of 11 tasks

Excessive handshakes in desktop client #6053

charlag opened this issue Oct 30, 2023 · 0 comments · Fixed by #6072
Labels
bug broken functionality, usability problems, unexpected errors desktop Desktop client related issues state:done meets our definition of done state:tested We tested it and are about to release it
Milestone
3.118.28

Comments

@charlag
Copy link
Contributor

charlag commented Oct 30, 2023 •
edited by ganthern

v3.118.12

We see many cases of users hitting "Bad Handshake Ratio" limits, especially while indexing. It seems like it could be related to using fetch() builtin from Node. Local testing shows that we indeed do a lot of handshakes and it seems like keepalive time is very short.

Tasks

  • Try to reproduce locally
  • Replace networking impl in ProtocolProxy
  • Make sure that socket timeout and idle timeout work correctly

Test notes

  • Can be done with tcpdump + wireshark. Start the capture, make some network requests, wait 20s, do more network requests. Check that after 20s pause the connection wasn't closed (no RST packets) and no new handshakes were made (no CLIENT HELLO packets). Check that after ~5min of idle the connection will be closed (RST packets, new handshake on new requests). UPDATE will not work without the server changes, connection will be closed after 20 seconds but not earlier.
    • FF
    • Chrome
    • Safari
    • Desktop (at least one platform)
  • Check that desktop apps can download big bodies slowly (overall request takes longer than 20s, perhaps with network connection throttling?)
  • Check that desktop apps time out on big downloads when there's no data sent for 20s (disable the connection and wait)
  • Check that mobile apps work
@charlag charlag added bug broken functionality, usability problems, unexpected errors desktop Desktop client related issues labels Oct 30, 2023
@charlag charlag self-assigned this Oct 30, 2023
charlag added a commit that referenced this issue Oct 30, 2023
@charlag
[desktop] Fix making excessive handshakes
3bbaa96 
fix #6053
charlag added a commit that referenced this issue Nov 2, 2023
@charlag
[desktop] Fix making excessive handshakes
f5dfb4e 
fix #6053
@charlag charlag mentioned this issue Nov 2, 2023
Merged
charlag added a commit that referenced this issue Nov 6, 2023
@charlag
[desktop] Fix making excessive handshakes
e59d589 
Since we switched to built-in fetch() as our implementation we noticed
that a number of users are making excessive TLS handshakes. We've found
that by default undici (that's powering node's built-in fetch()) has
low idle timeout and has no default limit for connections.

We are now explicitly using undici (also more recent version) with
explicitly configured dispatcher to adjust the http client parameters.

We should switch to HTTP2 once it stabilizes in undici.

fix #6053
charlag added a commit that referenced this issue Nov 6, 2023
@charlag
[desktop] Fix making excessive handshakes
39a330f 
Since we switched to built-in fetch() as our implementation we noticed
that a number of users are making excessive TLS handshakes. We've found
that by default undici (that's powering node's built-in fetch()) has
low idle timeout and has no default limit for connections.

We are now explicitly using undici (also more recent version) with
explicitly configured dispatcher to adjust the http client parameters.

We should switch to HTTP2 once it stabilizes in undici.

fix #6053
@murilopereirame murilopereirame self-assigned this Nov 7, 2023
ganthern pushed a commit that referenced this issue Nov 13, 2023
@charlag @ganthern
[desktop] Fix making excessive handshakes
a5adb2e 
Since we switched to built-in fetch() as our implementation we noticed
that a number of users are making excessive TLS handshakes. We've found
that by default undici (that's powering node's built-in fetch()) has
low idle timeout and has no default limit for connections.

We are now explicitly using undici (also more recent version) with
explicitly configured dispatcher to adjust the http client parameters.

We should switch to HTTP2 once it stabilizes in undici.

fix #6053
@ganthern ganthern added the state:done meets our definition of done label Nov 13, 2023
@ganthern ganthern added this to the 3.118.28 milestone Nov 13, 2023
ganthern pushed a commit that referenced this issue Nov 13, 2023
@charlag @ganthern
[desktop] Fix making excessive handshakes
4e4f66a 
Since we switched to built-in fetch() as our implementation we noticed
that a number of users are making excessive TLS handshakes. We've found
that by default undici (that's powering node's built-in fetch()) has
low idle timeout and has no default limit for connections.

We are now explicitly using undici (also more recent version) with
explicitly configured dispatcher to adjust the http client parameters.

We should switch to HTTP2 once it stabilizes in undici.

fix #6053
ganthern pushed a commit that referenced this issue Nov 13, 2023
@charlag @ganthern
[desktop] Fix making excessive handshakes
237febb 
Since we switched to built-in fetch() as our implementation we noticed
that a number of users are making excessive TLS handshakes. We've found
that by default undici (that's powering node's built-in fetch()) has
low idle timeout and has no default limit for connections.

We are now explicitly using undici (also more recent version) with
explicitly configured dispatcher to adjust the http client parameters.

We should switch to HTTP2 once it stabilizes in undici.

fix #6053
ganthern pushed a commit that referenced this issue Nov 13, 2023
@charlag @ganthern
[desktop] Fix making excessive handshakes
803bbf0 
Since we switched to built-in fetch() as our implementation we noticed
that a number of users are making excessive TLS handshakes. We've found
that by default undici (that's powering node's built-in fetch()) has
low idle timeout and has no default limit for connections.

We are now explicitly using undici (also more recent version) with
explicitly configured dispatcher to adjust the http client parameters.

We should switch to HTTP2 once it stabilizes in undici.

fix #6053
ganthern added a commit that referenced this issue Nov 14, 2023
@ganthern
vendor undici 5.27.2
e8aa8ca 
fix 803bbf0
close #6150
#6053
@ganthern ganthern mentioned this issue Nov 14, 2023
Merged
ganthern added a commit that referenced this issue Nov 14, 2023
@ganthern
vendor undici 5.27.2
0423a2f 
fix 803bbf0
close #6150
#6053
ganthern added a commit that referenced this issue Nov 14, 2023
@ganthern
vendor undici 5.27.2
6af48ab 
fix 803bbf0
close #6150
#6053
ganthern added a commit that referenced this issue Nov 14, 2023
@ganthern
vendor undici 5.27.2
1d4afc6 
fix 803bbf0
close #6150
#6053
@ganthern ganthern assigned ganthern and unassigned ganthern Nov 14, 2023
@ganthern ganthern added the state:tested We tested it and are about to release it label Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug broken functionality, usability problems, unexpected errors desktop Desktop client related issues state:done meets our definition of done state:tested We tested it and are about to release it
Projects
None yet
Milestone
3.118.28
Development

Successfully merging a pull request may close this issue.

[desktop] Fix making excessive handshakes tutao/tutanota
3 participants
@charlag @ganthern @murilopereirame