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

Backwards incompatible changes in plugin API #3209

Closed
Andriamanitra opened this issue Mar 24, 2024 · 3 comments · Fixed by #3211
Closed

Backwards incompatible changes in plugin API #3209

Andriamanitra opened this issue Mar 24, 2024 · 3 comments · Fixed by #3211

Comments

@Andriamanitra
Copy link
Contributor

4ffc220 removed some bindings that were previously available for plugins to use. This means that all plugins (such as my LSP plugin Andriamanitra/mlsp#1) that relied on them are broken on master.

I would suggest reverting the change as I don't see any harm in keeping these bindings around. If they are to be removed I think there should at least be a deprecation period to avoid disruptions for users. Especially so since the alternative was very recently introduced – as a plugin author I can't even update the plugin to use the alternative because it doesn't yet exist in the latest release (2.0.13), not to mention the distro packages which tend to lag behind by a few versions (for example Ubuntu LTS is still on 2.0.9).

@JoeKar
Copy link
Collaborator

JoeKar commented Mar 24, 2024

At least from my perspective (which doesn't need to be common) fully reverting it isn't what we want in the first place, since the intention was and is to provide a more abstracted API were we don't need to care about some specific micro internals take place in parallel.
But I give you fully right, that breaking APIs with no changing period isn't the best practice.

So my suggestion would be the reintroduction of these interfaces (partly revert), BUT with our own wrapper function where we can directly expose a deprecation warning. With this we should be save for the adaptation phase with the master as well as the current release.

Any objections?

dmaluka added a commit to dmaluka/micro that referenced this issue Mar 24, 2024
This reverts commit 4ffc220.

Reason for revert: some plugins happen to use raw Go timers via
time.AfterFunc(), in an unsafe way (without synchronizing their
async code with micro). Let them keep doing that for now, in an
unsafe way but at least without immediate crashes.

Fixes zyedidia#3209
@dmaluka
Copy link
Collaborator

dmaluka commented Mar 24, 2024

I've uploaded PR #3211 reverting 4ffc220. With #3211 your plugin should work as fine as it used to. I mean, as unsafe as it used to. From what I can see, your timer callback doesn't synchronize with the rest of your plugin, or with micro, whatsoever. What if e.g. activeConnections or openFiles changes in the meantime, while your time callback is running? It doesn't look like you update activeConnections in an atomic way, and so on. So I'd suggest to switch to the new (safe) API as soon as possible.

@dmaluka
Copy link
Collaborator

dmaluka commented Mar 24, 2024

I suppose you could also check at runtime if the new interface is available:

if type(micro.After) == "function" then
    -- use new API
else
    -- use old API
end

JoeKar pushed a commit that referenced this issue Mar 25, 2024
* Revert "Don't expose Go timers directly to lua"

This reverts commit 4ffc220.

Reason for revert: some plugins happen to use raw Go timers via
time.AfterFunc(), in an unsafe way (without synchronizing their
async code with micro). Let them keep doing that for now, in an
unsafe way but at least without immediate crashes.

Fixes #3209

* Add TODO about Go timers deprecation
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 a pull request may close this issue.

3 participants