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

Domain.join implementation #130

Merged
merged 7 commits into from Jun 8, 2017

Conversation

Projects
None yet
2 participants
@edwintorok
Contributor

edwintorok commented May 16, 2017

Domain.spawn now returns the pthread_t wrapped in a custom block.
Domain.join simply calls pthread_join.

pause had to be removed to avoid the thread hanging indefinetely.

edwintorok added some commits May 16, 2017

Implement Domain.join
Changes the type signature of 'Domain.spawn'

Signed-off-by: Edwin Török <edwin@etorok.net>
make bootstrap
Signed-off-by: Edwin Török <edwin@etorok.net>
Enable Domain.join
Fixes #89

Signed-off-by: Edwin Török <edwin@etorok.net>
@stedolan

This comment has been minimized.

Show comment
Hide comment
@stedolan

stedolan May 16, 2017

Contributor

Looks good! The call to pause is from a time when domains referred to each other's thread local variables (!), a poor design now fixed, so it can go away.

You do need some means of handling Domain.join being called multiple times, or in parallel by different domains - pthread_join has undefined behaviour if called twice. I'm not sure whether subsequent calls to Domain.join should be no-ops or raise an exception, but it should do something well-defined.

Contributor

stedolan commented May 16, 2017

Looks good! The call to pause is from a time when domains referred to each other's thread local variables (!), a poor design now fixed, so it can go away.

You do need some means of handling Domain.join being called multiple times, or in parallel by different domains - pthread_join has undefined behaviour if called twice. I'm not sure whether subsequent calls to Domain.join should be no-ops or raise an exception, but it should do something well-defined.

Show outdated Hide outdated byterun/domain.c
fixup! Implement Domain.join
Raise an exception if Domain.join is attempted multiple times.
Concurrent joins would be undefined otherwise.

Store the mutex in a non-movable C allocation, because copying mutexes is
undefined.
Also wrap pthread_join with blocking section.

Signed-off-by: Edwin Török <edwin@etorok.net>
@edwintorok

This comment has been minimized.

Show comment
Hide comment
@edwintorok

edwintorok May 17, 2017

Contributor

Thanks, I have added a mutex protected flag to detect multiple joins and exit early by raising an exception. Since I'm now storing a mutex inside the custom block I added another allocation (and a finalizer), I'm worried that OCaml's GC could move the custom block with the mutex in it which wouldn't be well defined.

Not sure whether I should just use caml_plat_lock/unlock instead of With_mutex, the mutex is only held for a short period of time to check a flag.

Contributor

edwintorok commented May 17, 2017

Thanks, I have added a mutex protected flag to detect multiple joins and exit early by raising an exception. Since I'm now storing a mutex inside the custom block I added another allocation (and a finalizer), I'm worried that OCaml's GC could move the custom block with the mutex in it which wouldn't be well defined.

Not sure whether I should just use caml_plat_lock/unlock instead of With_mutex, the mutex is only held for a short period of time to check a flag.

@stedolan

This comment has been minimized.

Show comment
Hide comment
@stedolan

stedolan May 17, 2017

Contributor

Thanks, I have added a mutex protected flag to detect multiple joins and exit early by raising an exception. Since I'm now storing a mutex inside the custom block I added another allocation (and a finalizer), I'm worried that OCaml's GC could move the custom block with the mutex in it which wouldn't be well defined.

You're right, you do need a means to prevent the mutex moving. The standard idiom here is to allocate the struct domain_thread yourself, using caml_stat_alloc, and then only store a pointer to that structure in the custom block. That way, the GC can move the custom block but won't move the structure.

Not sure whether I should just use caml_plat_lock/unlock instead of With_mutex, the mutex is only held for a short period of time to check a flag.

caml_plat_lock/unlock is fine. With_mutex includes some magic to ensure that the lock is released if an exception is thrown while the mutex is held, which is necessary for some hairy code in io.c. It's not needed here.

Contributor

stedolan commented May 17, 2017

Thanks, I have added a mutex protected flag to detect multiple joins and exit early by raising an exception. Since I'm now storing a mutex inside the custom block I added another allocation (and a finalizer), I'm worried that OCaml's GC could move the custom block with the mutex in it which wouldn't be well defined.

You're right, you do need a means to prevent the mutex moving. The standard idiom here is to allocate the struct domain_thread yourself, using caml_stat_alloc, and then only store a pointer to that structure in the custom block. That way, the GC can move the custom block but won't move the structure.

Not sure whether I should just use caml_plat_lock/unlock instead of With_mutex, the mutex is only held for a short period of time to check a flag.

caml_plat_lock/unlock is fine. With_mutex includes some magic to ensure that the lock is released if an exception is thrown while the mutex is held, which is necessary for some hairy code in io.c. It's not needed here.

fixup! fixup! Implement Domain.join
Use caml_stat_{alloc,free} and caml_plat_{lock,unlock}.

Signed-off-by: Edwin Török <edwin@etorok.net>
@edwintorok

This comment has been minimized.

Show comment
Hide comment
@edwintorok

edwintorok May 17, 2017

Contributor

Thanks, pushed a fixup commit.

Contributor

edwintorok commented May 17, 2017

Thanks, pushed a fixup commit.

caml_plat_event_init(&p.ev);
p.callback = caml_create_root(caml_promote(&domain_self->state, callback));
err = pthread_create(&th, 0, domain_thread_func, (void*)&p);
err = pthread_create(&d->th, 0, domain_thread_func, (void*)&p);

This comment has been minimized.

@stedolan

stedolan May 18, 2017

Contributor

If this returns an error, then d->th has not been initialised. The finaliser will run, and since joinable=true, it will try to pthread_detach(d->th).

I think joinable should be initialised to 0, and only set to 1 if pthread_create succeeds.

@stedolan

stedolan May 18, 2017

Contributor

If this returns an error, then d->th has not been initialised. The finaliser will run, and since joinable=true, it will try to pthread_detach(d->th).

I think joinable should be initialised to 0, and only set to 1 if pthread_create succeeds.

This comment has been minimized.

@edwintorok

edwintorok May 19, 2017

Contributor

Good point, pushed a fixup for that.

@edwintorok

edwintorok May 19, 2017

Contributor

Good point, pushed a fixup for that.

fixup! fixup! Implement Domain.join
joinable should be 0 if pthread_create fails

Signed-off-by: Edwin Török <edwin@etorok.net>
Assert(dp);
d = *dp;
Assert(d);
th = d->th;

This comment has been minimized.

@stedolan

stedolan May 20, 2017

Contributor

Here, d->th is accessed without holding the mutex d->m. It's not wrong, because d->th is readonly except in the finaliser, with which no code can race. However, the locking setup isn't obvious.

You should do one of:

  • Move this line after caml_plat_lock below so that d->m protects all accesses to d, or
  • Add a comment in the definition of struct domain_thread explaining that the mutex protects only joinable but not th, and that th is immutable once initialised.
@stedolan

stedolan May 20, 2017

Contributor

Here, d->th is accessed without holding the mutex d->m. It's not wrong, because d->th is readonly except in the finaliser, with which no code can race. However, the locking setup isn't obvious.

You should do one of:

  • Move this line after caml_plat_lock below so that d->m protects all accesses to d, or
  • Add a comment in the definition of struct domain_thread explaining that the mutex protects only joinable but not th, and that th is immutable once initialised.

This comment has been minimized.

@edwintorok

edwintorok May 20, 2017

Contributor

I've added a comment, I think it is clearer that way. Otherwise it could be confusing why copying the pthread_t needed the lock, but operating on it (with pthread_join) didn't.

@edwintorok

edwintorok May 20, 2017

Contributor

I've added a comment, I think it is clearer that way. Otherwise it could be confusing why copying the pthread_t needed the lock, but operating on it (with pthread_join) didn't.

fixup! fixup! fixup! Implement Domain.join
Add comment about locking setup.

Signed-off-by: Edwin Török <edwin@etorok.net>
@stedolan

This comment has been minimized.

Show comment
Hide comment
@stedolan

stedolan Jun 8, 2017

Contributor

Sorry for the delay, I've been quite busy the last while. This version looks good, thanks!

Contributor

stedolan commented Jun 8, 2017

Sorry for the delay, I've been quite busy the last while. This version looks good, thanks!

@stedolan stedolan merged commit b6182f7 into ocamllabs:master Jun 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@edwintorok edwintorok deleted the edwintorok:domainjoin branch Jun 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment