use random byte array instead of process_id and machine_id

[lrlna]

Hello o/

I made a PR last week to compat for WASM compilation, adding a WASM specific method to use random bytes arrays to substitute a call to libc and getting hostname. In the discussion there, @saghm pointed out MongoDB drivers are now transitioning to use 5 random bytes instead of process_id and machine_id (spec). This PR implements that.

In addition to using random bytes, this PR also:

• removes getter method for process_id (spec outlines that random value must not be accessible)
• removes process_id and machine_id entirely
• adds a check to travis for a WASM build

This would be probably a semver major in semver terms, but I am not sure how you'd want to handle that (or deprecation for that matter).

Thank you for your time!

[lrlna]

@kyeah i opened this new PR instead of the previous wasm-only one. thanks!

[lrlna]

@saghm would you have time to give this a review some time this week?

[kyeah]

hey @lrlna — super super sorry for the delay, thank you for waiting. This looks correct to specification (5-digit PRNG-generated number with OS-level entropy.)

Thank you for the change. I'll be more responsive next time :)

[kyeah]

Two notes:

1. Looks like cargo check --target wasm32-unknown-unknown is failing. Not entirely sure why at first glance, but if anyone has leads be my guest!

2. Regarding deprecation:

   This would be probably a semver major in semver terms, but I am not sure how you'd want to handle that (or deprecation for that matter).

   I think the ideal would have been a slow transition where we kept the old machine/process_id format and #[deprecated] the machine_id, process_id methods for a month or so before doing a major cut to the new format. /shrug

[lrlna]

@kyeah thanks so much for the review! \o/

1. the cargo error comes from the decimal crate that does the Decimal128 implementation. I completely did not test that PR against the wasm target. That crate does a lot of its heavy lifting through libc which is very not wasm compatible. Let me do some more looking into this and get back to you!

2. If you'd like to do deprecation, we might have to add a wrapper method so people still can be creating ObjectId with machine_id/process_id (and therefore retrieving it with the two public methods) or the new way of random bytes. Otherwise you can't really retrieve machine_id if the ObjectId wasn't ever created with it. Is that along the lines you were thinking? Or did you have another idea?

[kyeah]

@lrlna That's another way to do it! Or we could use random bytes only for WASM, and then make it the default everywhere after a deprecation period. I think it's unlikely that people are using the machine_id and process_id methods though, and we'll eventually need a major semver bump (or minor based on the lax versioning in the crates community), so I'd be fine doing a hard cut for this release.

I'll let @zonyitoo chime in if there's thoughts on how careful we want to be here w/ the public API.

[zonyitoo]

Of course, these changes have to be in a major version bump.

[lrlna]

hello! Sorry I am the one who's been quiet now!!

The reason travis wasn't passing is because Decimal128 library upstream is a wrapper around C, and is therefore not compatible with WASM. Although I am currently working on a Decimal128 implementation that doesn't involve using C code, it might take a bit more time. And it would be beneficial for this code to be in master regardless of whether it's WASM compatible or not. Once I fix up my new Decimal128 implementation, I can put this check back into travis, and we should be back in fully operational in WASM land 🎉

[saghm]

It looks like the nightly tests are failing due to the serde-tests subcrate specifying serde as a feature, which I think is a holdover from when we first introduced it in place of rustc-serialize. @kyeah can you confirm that removing the serde feature here is correct? I can open a PR for it assuming it's the right thing to do.

[saghm]

Pinging @kyeah again?

[kyeah]

@saghm yes I think that's right, thank you for the followup ping!

[lrlna]

@saghm let me know if you end up making a PR. I will rebase

[saghm]

Yep, I'll make a PR now. Thanks for the response Kevin!

[saghm]

Just opened #120; hopefully the CI will pass and then we can merge it

[saghm]

Tests are passing on it! @kyeah @zonyitoo any chance we could get that merged to unblock this?

[kyeah]

Merged into master, I'll let you merge/rebase @lrlna and then this should be good to go, thanks 🙏

[zonyitoo]

Merged. right? @kyeah

[zonyitoo]

One test is failed in nightly:

$ cd serde-tests && cargo test -v --no-fail-fast
    Updating crates.io index
error: failed to select a version for `bson`.
    ... required by package `serde-tests v0.1.0 (/home/travis/build/zonyitoo/bson-rs/serde-tests)`
versions that meet the requirements `*` are: 0.13.0

Hmm..

[zonyitoo]

Tell me if you think it is ready to be published to crates.io. @kyeah @saghm

[lrlna]

woo! Thanks for merging!

I thought @saghm 's fix should have cleared that error up? That seems to be a similar error that was on master before 🤔