search

rabbitmq / rabbitmq-common

Common library used by rabbitmq-server and rabbitmq-erlang-client
Other
66 stars 112 forks source link

Support the NEW_PID_EXT format introduced in OTP-22 #313

Closed essen closed 5 years ago

essen commented 5 years ago

In OTP-22 the Creation field has been increased to be 32 bits. For now we only need to handle it when using term_to_binary and parsing the result manually.

The relevant merge in OTP is at https://github.com/erlang/otp/commit/167c5ef4d804ab84979284ef4f572a69d6d9ddf8

michaelklishin commented 5 years ago

Cherry-picked to v3.7.x.

michaelklishin commented 5 years ago

@essen this breaks Dialyzer and our pipeline with it. I'm investigating but we may have to revert this to produce 3.7.14.

essen commented 5 years ago

I'm guessing it doesn't like the unreachable clause. I can submit a different solution later today.

michaelklishin commented 5 years ago

Disabling one type of checks for this function seemed appropriate, so I did in https://github.com/rabbitmq/rabbitmq-common/commit/d623532870899d66e9b171713c40dd9c810f72ac.

essen commented 5 years ago

Oh I didn't know we could disable these this way. Nice!

michaelklishin commented 5 years ago

@essen unfortunately this approach doesn't work for RabbitMQ. We build releases on OTP < 22 but users will eventually run them on OTP 22. I don't think we can require OTP 22 even for 3.8. So we'd have to go with the runtime code installation approach that is more dynamic than a compile-time constant but also doesn't have the overhead of a runtime condition (not that it matters for this specific function).

@dumbbell do we still have any examples of that pattern left in master?

essen commented 5 years ago

Right. Well alternatively you could decompose the match and accept both regardless of the version it was compiled in. This is probably simpler than recompile.

dumbbell commented 5 years ago

@michaelklishin: Here is an example of runtime recompilation: https://github.com/rabbitmq/rabbitmq-common/blob/v3.6.x/src/rand_compat.erl

But as @essen said, we may be able to use plain pattern matching regardless of Erlang version.

michaelklishin commented 5 years ago

Sounds good.

On Fri, 29 Mar 2019 at 15:49, Jean-Sébastien Pédron < notifications@github.com> wrote:

@michaelklishin https://github.com/michaelklishin: Here is an example of runtime recompilation: https://github.com/rabbitmq/rabbitmq-common/blob/v3.6.x/src/rand_compat.erl

But as @essen https://github.com/essen said, we may be able to use plain pattern matching regardless of Erlang version.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/rabbitmq/rabbitmq-common/pull/313#issuecomment-477985618, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAEQqlMYGPoUCuFTZ7TvDzteGvhhhv_ks5vbgvBgaJpZM4cK0Po .

-- Staff Software Engineer, Pivotal/RabbitMQ

dumbbell commented 5 years ago

We still have an issue: in rabbit_misc:compose_pid/4, we don't pay attention to the value of Cre:

compose_pid(Node, Cre, Id, Ser) ->
    <<131,NodeEnc/binary>> = term_to_binary(Node),
    binary_to_term(<<131,103,NodeEnc/binary,Id:32,Ser:32,Cre:8>>).

In CI, it happens that Cre looks like 1554471735 (an Epoch-based timestamp perhaps?). It does not fit into one byte obviously and the function throws a badarg exception.

It looks to be platform-specific. The failure happens on a 64-bit Debian VM. I can't reproduce locally on a 64-bit FreeBSD (using the exact same version/commit of Erlang master).

I'm currently reading the source code of Erlang to understand that value. I also plan to try to cluster Erlang 21 and Erlang 22 nodes (so mixed versions) to see if Erlang 22 adapts and generates creation value to remain compatible with other nodes.

essen commented 5 years ago

Hmm maybe just generate new format if Cre is above 255? I'd love to know what it's used for, I'll have to check.

dumbbell commented 5 years ago

Yes, that's a possibility I had in mind too, but I don't know yet why RabbitMQ wants to mess with PIDs. I need to understand if they are exchanged between nodes.

If Erlang 22 doesn't generate 32-bit creation values when connected to nodes without support for NEW_PID_EXT, then we should be fine with a simple check of Cre (and the generation of the appropriate PID).

michaelklishin commented 5 years ago

@dumbbell pids are used as identifiers for connections and Shovels. I detected this incompatibility in a Shovel test suite IIRC.

essen commented 5 years ago

It's used in the rabbit_mnesia_rename code that is used to "rename nodes in the Mnesia database". It's pure black magic. :)

dumbbell commented 5 years ago

After some testing with Erlang 19.2 and Erlang 22 nodes connected, the Erlang 19.2 node understands NEW_PID_EXT PIDs sent by the Erlang 22 node: using term_to_binary/1 and binary_to_term/1 works out-of-the-box.

And indeed, after checking Erlang source code, it seems NEW_PID_EXT was introduced in Erlang 19.0. So we can simply check the value of Cre and format the appropriate PID binary.

michaelklishin commented 5 years ago

Backported to v3.7.x.