search

zeromq / chumak

Pure Erlang implementation of ZeroMQ Message Transport Protocol.
Mozilla Public License 2.0
197 stars 47 forks source link

Avoid calling `gen_server:reply/2` with `{from, From}` #54

Closed siiky closed 1 year ago

siiky commented 1 year ago

There was some inconsistency in these files (not between different files but inside a single file). In some places the pending_recv field (or similar) was set to {from, From} and other times to From; and in pattern-matching too, some places pattern-matched on nil/none and {from, From}, others just From.

I noticed this first in chumak_req:terminate/2, where gen_server:reply/2 is called with the pending_recv field, which somewhere else in the file is set to {from, From}. From the gen_server:reply/2 docs I don't think this is accepted.

To make things consistent inside files but also across different files, I tried replacing all {from, From} tuples with From everywhere. Absence is marked by the symbols nil or none as before. I tried to fix pattern-matching orders in function clauses and cases but may have missed some.

I find it weird that this hasn't caused problems in the past; maybe this was/is dead code?

I ran rebar3 eunit -c and the only failed tests seemed to be related to cryptography (e.g. chumak_protocol_curve_test:messages_test/0: module 'chumak_protocol_curve_test') which shouldn't be affected by these changes. Still, wIll leave this PR in draft for a while during the project, though I don't use all socket types, to see if it breaks anything obvious.

drozzy commented 1 year ago

Would it be unreasonable to just fix the things that affect you directly? If you don't have time to test all the cases out, just leave those changes out. My thinking is if someone needs this functionality they can introduce a fix and test it at the same time.

siiky commented 1 year ago

That makes sense to me. Pushed a new commit with the changes for chumak_req.erl. Saved the original commit in case anyone may find it useful in the future.