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

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

Merged
merged 1 commit into from
Jun 12, 2023
Merged

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

merged 1 commit into from
Jun 12, 2023

Conversation

siiky
Copy link
Contributor

@siiky siiky commented Jun 12, 2023

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
Copy link
Member

drozzy commented Jun 12, 2023

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.

Replace `{from, From}` tuple with `From`. Absence is marked by the
symbol `nil` as before.
@siiky
Copy link
Contributor Author

siiky commented Jun 12, 2023

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.

@siiky siiky marked this pull request as ready for review June 12, 2023 18:11
@drozzy drozzy merged commit 2a0543f into zeromq:master Jun 12, 2023
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 this pull request may close these issues.

2 participants