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

Thread less autoupdate #1018

Merged
merged 9 commits into from
Dec 18, 2024

Conversation

kazu-yamamoto
Copy link
Contributor

@khibino I'm trying to merge your version of AutoUpdate.

  • Which module should export mkClosableAutoUpdate? (AutoUpdate or Internal?)
  • Could you implement mkAutoUpdateWithModify, too?
  • Any other concerns?

@khibino
Copy link
Contributor

khibino commented Dec 14, 2024

Could you implement mkAutoUpdateWithModify, too?

If my understanding is correct,
the action passed additionally to mkAutoUpdateWithModify will always be used
in place of the updateAction in UpdateSettings for update action calls starting from the second call.

Based on that premise, I implemented it as follows.

https://github.com/khibino/wai/compare/extend-thread-less~3..extend-thread-less

https://github.com/khibino/wai/tree/extend-thread-less

@khibino
Copy link
Contributor

khibino commented Dec 14, 2024

Which module should export mkClosableAutoUpdate? (AutoUpdate or Internal?)

Considering exporting the combination of mkAutoUpdateWithModify and mkClosableAutoUpdate,
it seems good to keep the exports from AutoUpdate simple,
and handle complex cases through exports from Internal.

On the other hand, while "thread-less" eliminates the thread leak problem when stopping auto-update midway,
there is a minor issue where timeout registration in GHC's TimeManager remains if mkClosableAutoUpdate is not used.

@kazu-yamamoto
Copy link
Contributor Author

@khibino I cherry-picked your commits.
Do you agree with merging this branch as is?

@khibino
Copy link
Contributor

khibino commented Dec 16, 2024

Yes. I think that's good.

@kazu-yamamoto
Copy link
Contributor Author

@khibino The current implementation cannot be built on Windows because GHC.Event cannot be imported.

@kazu-yamamoto
Copy link
Contributor Author

@khibino I will use the old implementation for Windows.

Windows does not provide GHC.Event, sigh.
@kazu-yamamoto
Copy link
Contributor Author

@khibino CI passes. Would you give one more look?

@kazu-yamamoto kazu-yamamoto merged commit b9cf8ed into yesodweb:master Dec 18, 2024
14 checks passed
@kazu-yamamoto kazu-yamamoto deleted the thread-less-autoupdate branch December 18, 2024 03:38
@kazu-yamamoto
Copy link
Contributor Author

Merged and released.
Thanks!

@khibino
Copy link
Contributor

khibino commented Dec 18, 2024

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.

2 participants