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

Make Settings Send + Sync #117

Closed
wants to merge 2 commits into from
Closed

Conversation

jyn514
Copy link
Contributor

@jyn514 jyn514 commented Dec 21, 2021

This works by requiring ElementContentHandlers to be Send + Sync. Making Settings Send + Sync can be useful if you want to reuse the same settings across multiple rewriters in different threads.

This is a breaking change.

Helps with #82, but I'd prefer to avoid merging it until I hear back from the author about why they need this.

This makes `Settings` Send + Sync, which can be useful if you want to reuse
the same settings across multiple rewriters in different threads.

This is a breaking change.
@jyn514 jyn514 requested a review from a team as a code owner December 21, 2021 20:49
@kornelski
Copy link
Contributor

Do callbacks really need to be Send + Sync? This may be an annoying limitation, especially Sync.

@jongiddy
Copy link
Collaborator

This causes problems with existing code if Rc<RefCell<_>> is used to share data between multiple closures. Such structures are required for keeping state over multiple closures, especially when using on_end_tag with its closure within a closure (e.g. https://github.com/jongiddy/strictly-data/blob/b5ff138149cfb0fd763fc1c44639c81388f5ebd2/src/extract.rs#L493-L499).

If this change is merged, would such structures need to be replaced with Arc<Mutex<_>> even if threading is not used?

@jyn514
Copy link
Contributor Author

jyn514 commented Jan 4, 2022

If this change is merged, would such structures need to be replaced with Arc<Mutex<_>> even if threading is not used?

It would, yes.

Ok, I think I'm convinced this isn't the right way to fix this.

@jyn514 jyn514 closed this Jan 4, 2022
@jyn514 jyn514 deleted the jnelson/send-sync-settings branch January 4, 2022 23:33
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.

3 participants