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

Append *default-special-bindings* set from elsewhere on thread creation #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

leetwinski
Copy link

Follow up on this discussion in stmx repository.
We found out that *default-special-bindings* special variable gets overwritten by kernel bindings in with-thread macro on thread creation.

The patch solves this, appending the possibly existing values to kernel bindings. Fixes the interoperability issue with stmx and possibly other libraries, relying on those thread bindings.

@cosmos72
Copy link

cosmos72 commented Oct 27, 2021

I am stmx author, and I confirm that removing some (or all) existing conses (symbol . form) from bordeaux-threads:*default-special-bindings*, as lparallel's (with-thread) macro in src/thread-util.lisp does,

breaks any library (including mine) that add conses to it and rely on them to remain present.

This patch looks correct to me: many libraries have reasons to add conses to bordeaux-threads:*default-special-bindings*, including at least both stmx and lparallel. But I see no reasonable use case where a library should remove conses added by someone else.

Also, bordeaux-threads docs explicitly state

The effect of mutating this list is undefined, but earlier forms take precedence over later forms
for the same symbol, so defaults may be overridden by consing to the head of the list.

so even in case a symbol already appears bordeaux-threads:*default-special-bindings*, the correct approach for overriding it is to prepend another cons containing a different value for it.

@sabracrolleton
Copy link

Does anyone have any problems with this pull request?

@cosmos72
Copy link

As it can (hopefully) be deduced from my post above, I am definitely in favor of merging this pull request.

@leetwinski
Copy link
Author

leetwinski commented Oct 20, 2022

I couldn't locate anyone able to approve and/or merge this. Also tried to reference it in sharplispers/lparallel. Also no luck.

@mdbergmann
Copy link

mdbergmann commented Oct 20, 2022

Last time I had luck make people aware in Reddit.

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.

4 participants