Skip to content

Comments

Thread safety for lhash documented.#4429

Closed
paulidale wants to merge 1 commit intoopenssl:masterfrom
paulidale:lh-nolock
Closed

Thread safety for lhash documented.#4429
paulidale wants to merge 1 commit intoopenssl:masterfrom
paulidale:lh-nolock

Conversation

@paulidale
Copy link
Contributor

Document that lhash isn't thread safe under any circumstances and indicate the level of locking required for various operations. Remove the lock and atomics from the lhash code. These we're not complete
or adequate.

This replaces both #4418 and #4427 which will be closed unmerged.

  • documentation is added or updated

Copy link
Contributor

Choose a reason for hiding this comment

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

I sense certain ambiguity here. Thing is that while read lock is sufficient for OPENSSL_LH_stats itself, it effectively depends on write locks at retrieve time. And I fail to see this relation emphasized. Would something along following be more appropriate? "... under a read lock, unless accurate usage statistic is desired. In which case a write lock [for retrieve operations] should be used, for OPENSSL_LH_stats read lock suffices." Idea is to "break" it into two "causality" chains...

Copy link
Contributor

Choose a reason for hiding this comment

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

This actually emphasizes my point quite vividly. It gives impression that read lock at OPENSSL_LH_stats is sufficient for consistent output. In this case would it be appropriate to omit "or inconsistent" and simply refer to OPENSSL_LH_COMPFUNC(3), and even more specifically to NOTE section?

@paulidale
Copy link
Contributor Author

The POD wording has been improved as per the suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not exactly happy about "always". Even though we can't deny user looking at inaccurate statistics, we probably shouldn't encourage it. I mean I feel that last sentence effectively does the latter. But on the other hand it might as well happen that I'm the only one who gets that impression. In other words I'd omit "always", would anybody else? I also wonder what effect would "the" have in "For output of the usage statistics". Would it be interpreted rather as "for output of [abovementioned accurate] usage statistics"? If so, it would also be an improvement in my ears...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both suggestions incorporated and squashed.

Copy link
Contributor

Choose a reason for hiding this comment

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

typo: retreve -> retrieve

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Got it correct two lines above :(

@paulidale
Copy link
Contributor Author

Documentation changes done now.

@paulidale
Copy link
Contributor Author

Seeking an OMC tick.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, there's a notation, so you could simply have See L<OPENSSL_LH_COMPFUNC(3)/NOTE> for more details ...

Copy link
Member

Choose a reason for hiding this comment

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

... and for HTML output, that'll make for a link to the correct anchor as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks.
I've learned my thing for the day :)

indicate the level of locking required for various operations.

Remove the lock and atomics from the lhash code.  These we're not complete
or adequate.

Refer to openssl#4418 and openssl#4427 for details.
@paulidale
Copy link
Contributor Author

ping?

Copy link
Contributor

@richsalz richsalz left a comment

Choose a reason for hiding this comment

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

Looks like you addressed @levitte's issue, so +1 from me

Copy link
Contributor

@kaduk kaduk left a comment

Choose a reason for hiding this comment

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

Thank you for putting this together.

levitte pushed a commit that referenced this pull request Oct 8, 2017
indicate the level of locking required for various operations.

Remove the lock and atomics from the lhash code.  These we're not complete
or adequate.

Refer to #4418 and #4427 for details.

Reviewed-by: Rich Salz <[email protected]>
Reviewed-by: Ben Kaduk <[email protected]>
Reviewed-by: Andy Polyakov <[email protected]>
(Merged from #4429)
@paulidale
Copy link
Contributor Author

Merged, thanks.

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.

6 participants