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

type mismatch causes lwt_unix job hangs #277

Closed
stijn-devriendt opened this issue Oct 17, 2016 · 0 comments
Closed

type mismatch causes lwt_unix job hangs #277

stijn-devriendt opened this issue Oct 17, 2016 · 0 comments

Comments

@stijn-devriendt
Copy link
Contributor

To reproduce:
In Lwt_unix.ml, change the following::

  let current_notification_id = ref (0x7FFFFFFF - 1000)

Then run the tests. Test lwt_io_non_block:"many read file" will hang.

Root cause is a type mismatch in the C-code. notification_id is used as int (Int_val) in the job code and long (Long_val) in other cases. The result is that when the notification ID reaches 2^31, notifications from unix jobs are lost. This is because the 0x80000000L is cast to int, yielding INT_MIN. Later on, this value is cast back to long with sign extension (0xFFFFFFFF80000000) to generate the notification. This value is not found in the notifications hashtable, causing loss of notifications and thus a hang of the affected Lwt tasks.

All uses of notification id's should be long to match the ocaml value's size.

aantron added a commit that referenced this issue Oct 17, 2016
This patch types all C notification id fields, arguments, and variables
at type intnat, which is exposed by caml/mlvalues.h, and ultimately
comes from caml/config.h. intnat is the C data type used by the OCaml
runtime for storing OCaml ints. Its definition is selected according to
the machine word size and the C compiler detected during configuration
of OCaml.

The corresponding conversion macros between intnat and value are
Long_val and Val_long.

Fixes #277.
aantron added a commit that referenced this issue Oct 18, 2016
This patch types all C notification id fields, arguments, and variables
at type intnat, which is exposed by caml/mlvalues.h, and ultimately
comes from caml/config.h. intnat is the C data type used by the OCaml
runtime for storing OCaml ints. Its definition is selected according to
the machine word size and the C compiler detected during configuration
of OCaml.

The corresponding conversion macros between intnat and value are
Long_val and Val_long.

Fixes #277.
aantron added a commit that referenced this issue Oct 18, 2016
Before this patch, on 64-bit systems, C code stored 63-bit notification
ids in 32-bit integer fields, truncating them.

This patch types all C notification id fields, arguments, and variables
at type intnat, which is the C data type used by the OCaml runtime for
storing OCaml ints. Its definition is selected during configuration of
OCaml according to the machine word size and C compiler. It has the
proper bit width for each system. It is exposed by caml/mlvalues.h, and
ultimately comes from caml/config.h.

The corresponding conversion macros between intnat and value are
Long_val and Val_long.

The initial value of Lwt_unix.current_notification_id is now set so that
truncating a notification id would cause tests to fail as described
in #277.

Fixes #277.
aantron added a commit that referenced this issue Oct 18, 2016
Before this patch, on 64-bit systems, C code stored 63-bit notification
ids in 32-bit integer fields, truncating them.

This patch types all C notification id fields, arguments, and variables
at type intnat, which is the C data type used by the OCaml runtime for
storing OCaml ints. Its definition is selected during configuration of
OCaml according to the machine word size and C compiler. It has the
proper bit width for each system. It is exposed by caml/mlvalues.h, and
ultimately comes from caml/config.h.

The corresponding conversion macros between intnat and value are
Long_val and Val_long.

The initial value of Lwt_unix.current_notification_id is now set so that
truncating a notification id would cause tests to fail as described
in #277.

Fixes #277.
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

No branches or pull requests

1 participant