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

Support for 32bit platforms #63

Closed
epoberezkin opened this issue Mar 30, 2023 · 9 comments
Closed

Support for 32bit platforms #63

epoberezkin opened this issue Mar 30, 2023 · 9 comments
Assignees

Comments

@epoberezkin
Copy link

Client code compiled for ARMv7a (32bit) fails on receiving streamed request bodies (sending mostly works, it only inconsistently gets stuck on 32bit, so sending may also be affected in some cases).

This is quite unfortunate as we just released v4.6 of SimpleX Chat that supports ARMv7a on Android, and v4.6.1 about to be released includes the experimental support for file transfers that uses http2, and v5.0 planned in April was going to switch to using it for all transfers...

It's not 100% confirmed that the root cause is http2, but it doesn't seem device or OS specific - the same device works with arm64-v8a code, and doesn't allow receiving files on armv7a - it receives some bodies, and gets stuck on some others. Also the rest of the stack works ok (TCP, TLS, encryption, there is very little code that's specific to receiving files that's not part of http2 library), there are no issues with 64 bit code.

My hypothesis is that there is some platform-specific numbers (Int or Word) that are either overflown sooner on 32bit platform or encoded differently when they are sent to the server.

We will try to add some event logging, so may add more Information here a bit later – debugging haskell code on Android is hard.

@kazu-yamamoto please help figuring it out!

@kazu-yamamoto kazu-yamamoto self-assigned this Mar 31, 2023
@kazu-yamamoto
Copy link
Owner

I cannot find any 64bit values in RFC 9113.

But I found this code in tls: https://github.com/haskell-tls/hs-tls/blob/master/core/Network/TLS/Handshake/Common13.hs#L346

I don't know whether or not this code is relating to this issue.

@kazu-yamamoto
Copy link
Owner

The calculation for HTTP/2 flow control is done around 2^31 - 1.
This is probably unsafe.
We don't have to use this big number to disable the flow control.
For instance, 2^30 (1 giga bytes) is large enough.

@epoberezkin
Copy link
Author

@kazu-yamamoto thanks a lot for looking into it!

But I found this code in tls: https://github.com/haskell-tls/hs-tls/blob/master/core/Network/TLS/Handshake/Common13.hs#L346

This actually might be safe, as it has Word32, rather than Word. Also we had no issues with TLS itself - the messaging works ok on 32 bit platform.

The calculation for HTTP/2 flow control is done around 2^31 - 1.
This is probably unsafe.

We could just do the calculation with the same number but without using plain Int/Word and instead using Int64 or Word32 - then it should compile in the same way to both 32 and 64 bit platforms, no need to change the number.

Will you be able to make the change or point me to where this calculation is in the code - I would do a PR?

Thank you 🙏

@kazu-yamamoto
Copy link
Owner

I'm planning to change WindowSize using newtype and introduce safe calculation mechanism by checking boundary, etc.

My concern is that #62 probably suggests disabling flow control in this way is a bad idea. I need to consider hard.

@epoberezkin
Copy link
Author

alright, will test as soon as there is a PR!

We can connect in the chat - I would see messages faster there.

@epoberezkin
Copy link
Author

reading #62 - it seems like it has to be solved independently, otherwise it can cause server problem.

@epoberezkin
Copy link
Author

I cannot yet confidently say that http2 library does not have this problem - it might - but replacing all Int's with Int32's when running on 64 bit platform didn't reproduce the issue... Whilst replacing all Int's with Int32's in our code did :)

So we certainly do have problem in our code, and I've managed to both reproduce the issue that happens on 32 bit platform, and to fix it by making a computation 64bit... I'll close the issue once I've confirmed it is all working on 32 bit platform - currently I am testing on 64bit.

@epoberezkin
Copy link
Author

alright, it's working! it's embarrassing :)

Thanks a lot for improving it! Currently using what's on master branch - is that right?

@kazu-yamamoto
Copy link
Owner

The master branch is the most stable with #66 (proper initial window size) and #67 (fixing race).
I will release version 4.1.2 after looking into the result of field tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants