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

fix(api): Keep command keys stable across boots #12581

Merged
merged 6 commits into from
Apr 28, 2023
Merged

fix(api): Keep command keys stable across boots #12581

merged 6 commits into from
Apr 28, 2023

Conversation

SyntaxColoring
Copy link
Contributor

Overview

Fixes RSS-215. See that ticket for details.

Test Plan

I've reproduced the original problem on an OT-3, and run this branch on an OT-3 and confirmed that it fixes it.

Changelog

  • Do not use Python's built-in hash() function to compute Protocol Engine command keys, because it changes across runs of the Python interpreter. Instead, compute the hash "manually" with hashlib.md5.
  • Add tests for this function. The tests can't cover this particular bug, unfortunately, but we should have them anyway.

Review requests

Risk assessment

Low.

@SyntaxColoring SyntaxColoring requested a review from a team as a code owner April 27, 2023 22:39
@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Merging #12581 (c4b404c) into edge (bb34bd7) will not change coverage.
The diff coverage is n/a.

❗ Current head c4b404c differs from pull request most recent head 0cea0d9. Consider uploading reports for the commit 0cea0d9 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             edge   #12581   +/-   ##
=======================================
  Coverage   73.59%   73.59%           
=======================================
  Files        2267     2267           
  Lines       62341    62341           
  Branches     6592     6592           
=======================================
  Hits        45881    45881           
  Misses      14886    14886           
  Partials     1574     1574           
Flag Coverage Δ
notify-server 89.13% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yup. If we want there's a magic env var you can set to fix the per-proc hash seed but... they do this basically exclusively to cause this issue and I think it's a good thing, so doing our own hashing is a good call.

Do we use hash() anywhere else?

@sfoster1
Copy link
Member

Oh @SyntaxColoring if you want to test this exact behavior you could write a little script that calls this code and then run it with a python subprocess twice and check the outputs

@SyntaxColoring
Copy link
Contributor Author

Do we use hash() anywhere else?

It looks like we have a few places, but they look legitimate: using hash() to implement __hash__(self).

Copy link
Contributor

@TamarZanzouri TamarZanzouri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! thank you!

@SyntaxColoring
Copy link
Contributor Author

Oh @SyntaxColoring if you want to test this exact behavior you could write a little script that calls this code and then run it with a python subprocess twice and check the outputs

I was hoping this would work with a pair of concurrent.futures.ProcessPoolExecutors, but I think they share the same hash seed because they're forked from the same parent process.

It's probably possible to get test coverage with something like what you said, where we explicitly start a Python subprocess ourselves. From a call with @TamarZanzouri and @jbleon95, we don't think that's worth it at the moment, though.

@SyntaxColoring SyntaxColoring merged commit fb41f45 into edge Apr 28, 2023
@SyntaxColoring SyntaxColoring deleted the RSS-215 branch April 28, 2023 14:50
@SyntaxColoring SyntaxColoring restored the RSS-215 branch April 28, 2023 16:24
@SyntaxColoring SyntaxColoring deleted the RSS-215 branch February 29, 2024 22:12
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.

3 participants