lemunozm / message-io

Fast and easy-to-use event-driven network library.
Apache License 2.0
1.12k stars 75 forks source link

32-bit OS #41

Closed ray33ee closed 3 years ago

ray33ee commented 3 years ago

message-io doesn't seem to work on 32-bit systems. Some of the constants in resource_id.rs (namely RESOURCE_TYPE_BIT and ADAPTER_ID_MASK_OVER_ID) overflow in 32-bit systems.

Perhaps the constants can be updated to make use of std::mem::size_of, or the type changed from usize to u64?

lemunozm commented 3 years ago

You're right @ray33ee

I think that the fix is easy, I will upload it as soon as I can.

Thanks for advising!

lemunozm commented 3 years ago

From version 0.9.4, message-io should be work fine in 32bits arch.

I added CI jobs for x32 and they are passing: https://github.com/lemunozm/message-io/actions/runs/611332845

ray33ee commented 3 years ago

Thanks for getting back to soon!

I haven't been able to try out 0.9.4 yet, but it still looks like there would be an issue, this time with BASE_VALUE_MASK. Since the literal 0xFFFFFFFFFFFFFF00 won't fit within 32-bits, I would expect the compiler to produce an error. And indeed when I copy the declaration of BASE_VALUE_MASK into an empty Rust project it throws a 'literal out of range for usize' compiler error.

Can you confirm that the jobs are running at x32? Specifically getting the size of `usize' and confirming that is is testing it is 4 bytes long?

I think changing the literal to 0xFFFFFF00 should fix the issue once and for all

lemunozm commented 3 years ago

Hi @ray33ee,

The problem with the literal 0xFFFFFF00 is that only left available different 2^24 connections, that for some server use-case, it can be small. I want that the 64bits machines can use all their potential.

The jobs are running under x32 OS, so I understand that it should run the binary as 32bits. Anyway, it's true the compiler behaviour you suggest. I think that the following definition of constant should work. I will add it.

const BASE_VALUE_MASK: usize = 0xFFFFFFFFFFFFFF00_u64 as usize;

I'm not totally sure about how rust traits a literal usize that exceeds 32bits in x32 but I thought that it was clamped. Could you confirm that it is a compiler error? If so, I have something wrong with the x32 jobs.

If you are not using yet version 0.9 of message-io, I am going to upload tomorrow a 0.10 with some API modifications, in case you prefer to wait.

ray33ee commented 3 years ago

That makes sense!

Yeh I'm not sure about the jobs, I think it would be worth checking the actual size of usize to confirm.

If you want a real platform to test x32 on, a Raspberry Pi is a great choice (It's what I'm using and it's how I found the issue in the first place).

I've just tried that definition and it compiles, and it seems to behave as expected. My only concern is what happens if one device running 32-bit OS tries to compare the Endpoint with another device running 64-bit OS. (lets say you have two clients with different architectures) what would happen then? It just seems to make sense for all devices to store the id in exactly the same way, and convert it to usize only when needed.

I think one way around this would be to store the id field as a u64, then convert it to a usize only when it is needed (for use by mio, for example)

lemunozm commented 3 years ago

The problem with the raspberry is that I can't test automatically if some change breaks something in 32 bits unless I compile it manually for it. But I'll end up buying one!

Regarding the Endpoint comparison, the Endpoint is something local to your Network instance. The inner ResourceId is used to identify the client resource in your own system, it's not an identitication of the client itself. For that, the endpoint is something that must not be sent and used outside of the Network scope of who create it. Instead, you can share the endpoint.addr() that contains the public address of the client to connect with it.

There is something wrong with the x32 jobs because I add if std::mem::size_of::<usize>() == 4 { panic!() } and all the tests have been passed. I would investigate.

ray33ee commented 3 years ago

Yeh it's not going to work for automatic testing, but it might be something to consider if you need cheap x32 hardware.

Right of course! That makes sense, just wanted to make sure that there wouldn't be any issues. I think changing the declaration to 0xFFFFFFFFFFFFFF00_u64 as usize will do it

lemunozm commented 3 years ago

Upload version 0.10.0 that contains the fix (among others). Please, if you found any other issue with x32 systems tell me.

Also, since the last versions of message-io have several API breaks, if you need help updating some parts, feel free to put another issue. An important tricky detail of this last version is that Transport::Tcp has been renamed to Transport::FramedTcp. You probably want to use Transport::FramedTcp because now the currently Transport::Tcp of 0.10 works differently.

Which model or RaspberryPi are you using? 😋

ray33ee commented 3 years ago

It might be a while before I can test it, using any version 0.7.0 or above gives an error about OpenSSL (still investigating it, probably due to the Pi installation) but when it's sorted I will let you know!

I haven't updated to 0.10.0 just yet (its quite a change from 0.6.0!

I didn't want to change it over until I was sure there would be no more major changes, should I expect any major API breaks anytime soon or is it safe to change to 0.10.0?

And it's a Pi 4 Model B. Even though it is a 64-bit processor, most OSs only support 32-bit at the moment (although there are unstable 64-bit versions about)

lemunozm commented 3 years ago

I had the same issue cross-compiling in 32 in order to fix the problem in my computer. I could not fix it (although I did not investigate so much, it seems some environment config). Do you think It is due to the message-io that adds some conflict?

I know that recently there were a lot of API changes, sorry. I have wanted to incorporate iteratively some ideas coming from other projects and polish them to left the easier API while the library increases its usage in new scenarios. At this moment of the roadmap, the main break changes are done. I hope to only perform fixes and non-break additions to the current state, so I think that the API now is stable enough.

Thanks, I just was seeing just that RasbperryPi version as the best power-cost balanced.

sigmaSd commented 3 years ago

Hello,

tungstenite-rs requires openssl, so you'll either have to specify 32bit openssl lib path when cross-compiling, or use tungstenite-rs rustls-tls feature flag which will use rust implementation instead.

lemunozm commented 3 years ago

I was thinking about it, and to avoid this "problem" to an user that wants to cross-compile message-io, I will associate the transports to features. In this way, If you don't want to use Websocket, for example, you don't have to compile it and deal with openssl.

lemunozm commented 3 years ago

message-io from version 0.10.1 allows building with the transport you want specifying by features. By default it uses all the transports, but, for example, if you add:

message-io = { version = "0.10", default-features = false, features = ["tcp", "udp"] }

You only will compile the dependencies needed for tcp and udp, avoiding compiling OpenSSL in 32 bits if you do not need websockets.

lemunozm commented 3 years ago

Since it works now in 32bits I close the issue. Please if you find another issue be free to create a new one.