Skip to content

Conversation

@Cartmanishere
Copy link
Contributor

Hi,

This is an attempt to fix issue around delayed middleware loading.
The relevant discussion is present at #533

Instead of adding the handler delay to the delayed-handlers atom in the macro, this takes care of adding
it in the atom through a function.

Fixes:
#447
#533
#657

@Cartmanishere
Copy link
Contributor Author

Test for this are failing. The failures seem related to cljs and are not caused by the changes in this PR.

@bbatsov
Copy link
Member

bbatsov commented Nov 23, 2020

Did you manage to confirm that the suggested fix solves the race conditions? //cc @vspinu

Also - this needs a changelog entry.

Test for this are failing. The failures seem related to cljs and are not caused by the changes in this PR.

Yeah, there's a ticket for this #678 but I haven't been able to find time to investigate things further.

@Cartmanishere
Copy link
Contributor Author

Cartmanishere commented Nov 24, 2020

This is not a race condition per se. The problem is that we are relying on compile time side effects to be present at runtime. This works in normal scenario but breaks when the middlewares are compiled from within an application.

So to fix this, instead of relying on compile time side effects, we handle it at runtime. This fixes the issue. #447 has a reproducible example against which you can check. I have verified the same.

@bbatsov bbatsov merged commit 9d977b4 into clojure-emacs:master Nov 24, 2020
@bbatsov
Copy link
Member

bbatsov commented Nov 24, 2020

Got it. Thanks for tackling this! 🙇‍♂️ I'll cut a new release right away, as that is an important fix.

@Cartmanishere Cartmanishere deleted the fix-aot branch November 24, 2020 14:14
@timvisher
Copy link

This is seriously exciting. 😂

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