Skip to content

Don't automatically unlock/lock I/O channels managed from C#11356

Merged
xavierleroy merged 2 commits intoocaml:trunkfrom
xavierleroy:dont-lock-unmanaged-channels
Jul 1, 2022
Merged

Don't automatically unlock/lock I/O channels managed from C#11356
xavierleroy merged 2 commits intoocaml:trunkfrom
xavierleroy:dont-lock-unmanaged-channels

Conversation

@xavierleroy
Copy link
Copy Markdown
Contributor

@xavierleroy xavierleroy commented Jun 24, 2022

I/O channels allocated from C are entirely managed by the C code, including locking and unlocking, so no automatic unlock/lock should take place in check_pending.

Currently, all channels allocated from C are used in a single-threaded manner and don't need to be locked. However, they need to be locked in advance to avoid run-time errors when unlocking in check_pending. This is unpleasant and a source of potential errors, as in #11065 .

This PR turns off the unlock/lock dance in check_pending for channels allocated from C. I don't think there's any adverse effect since these channels are not accessible to OCaml code that could be run by finalizers or signal handlers. As a consequence we can remove a few Lock/Unlock pairs that were introduced just to please check_pending.

Cc: @damiendoligez @sadiqj @stedolan

@xavierleroy
In check_pending, don't assume channels allocated and used from C are…
de7950f 
… locked

Channels allocated from C are entirely managed by the C code,
including locking and unlocking, so no automatic unlock/lock should
take place.

Currently, all channels allocated from C are used in a single-threaded manner
and don't need to be locked.  However, they need to be locked in advance
to avoid run-time errors when unlocking in check_pending.

This is unpleasant and a source of potential errors: locking also sets
the last_channel_locked state component, causing surprise unlocks when
an exception is raised from C.
@xavierleroy
Remove unnecessary locking on I/O channels created and used from C
33afbe7 
They are used in a single-threaded manner.
@xavierleroy xavierleroy changed the title Dont lock unmanaged I/O channels Don't automatically unlock/lock unmanaged I/O channels Jun 24, 2022
@xavierleroy xavierleroy changed the title Don't automatically unlock/lock unmanaged I/O channels Don't automatically unlock/lock I/O channels managed from C Jun 24, 2022
@xavierleroy xavierleroy added this to the 5.0 milestone Jun 24, 2022
dra27
dra27 approved these changes Jun 24, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@dra27 dra27 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM - is it possibly worth a comment in the two places noting that the channel is not locked (as in debugger.c)?

@xavierleroy xavierleroy mentioned this pull request Jun 27, 2022
Merged
@dra27
Copy link
Copy Markdown
Member

dra27 commented Jun 30, 2022

Apropos damiendoligez#7 (comment), I grep'd opam for caml_{open,close}_descriptor and got:

  • ANSITerminal, delimcc, gsl, ocamlcc, ocsfml (copies of io.h in their sources)
  • ocamlspot, planck, typerex-lldb (symbols listed in a profiling trace)

I also grep'd for the Lock or Unlock macros, that only threw up core_unix, which is correctly using those with OCaml-allocated channels. In fact, ignoring files which are clearly third-party or just not C, our Lock and Unlock macros really don't seem to be used "in the wild".

At least from opam-repository, I'd tentatively conclude that the C interface for channels is not used much (it's also guarded by CAML_INTERNALS).

@xavierleroy
Copy link
Copy Markdown
Contributor Author

xavierleroy commented Jul 1, 2022

At least from opam-repository, I'd tentatively conclude that the C interface for channels is not used much (it's also guarded by CAML_INTERNALS).

Thanks for having checked. Quoting from the discussion on the other PR:

You can still Lock and Unlock your channels from C like you must do in 4.12 - 4.14 (if I'm not mistaken), but you can also elect not to Lock and Unlock provided the channel is used in a single-threaded manner, like you used to do in 4.11 and before. So, backward compatibility is improved !

It's the change in 4.12 that could have broken other libraries, by forcing them to add Lock/Unlock operations even for single-threaded usage. The change in this PR can only "un-break" these libraries.

@xavierleroy xavierleroy merged commit 9f4e887 into ocaml:trunk Jul 1, 2022
xavierleroy added a commit to xavierleroy/ocaml that referenced this pull request Jul 1, 2022
@xavierleroy
Remove unnecessary locking on I/O channels used by the debugger
2e4e8a7 
They are used in a single-threaded manner.
Locking is no longer necessary since ocaml#11356.

Also remove the recently-added caml_channel_reset_last_locked() function
which is no longer needed.
@xavierleroy
Copy link
Copy Markdown
Contributor Author

xavierleroy commented Jul 1, 2022

Cherry-picked to 5.0 : 34b694d

xavierleroy added a commit that referenced this pull request Jul 1, 2022
@xavierleroy
Merge pull request #11356 from xavierleroy/dont-lock-unmanaged-channels
34b694d 
Don't automatically unlock/lock I/O channels managed from C

(cherry picked from commit 9f4e887)
@xavierleroy xavierleroy deleted the dont-lock-unmanaged-channels branch October 12, 2022 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dra27 dra27 dra27 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

5.0

Development

Successfully merging this pull request may close these issues.

2 participants

@xavierleroy @dra27