Feature/#1 #19 - Make the plugin async

monovertex commented 6 years ago

Purpose

In its current state, the plugin is blocking, creating a pretty bad user experience, as ImportJS can hang, especially when starting the daemon and even more so on large JS projects.

As requested in #1, this PR makes the plugin async. It also fixes the issue in #19, by guarding for the possible exceptions and restarting the daemon when that happens.

Closes #1 Closes #19

Approach

In order to make the pipe readline method non-blocking, I used a separate thread with an attached queue that receives all the messages in the process' stdout pipe, as seen here.

That was not enough, as we still need to poll the queue itself for messages, when we send commands to the daemon. In order to achieve that I used sublime's set_timeout_async, which allowed me to create a non-blocking timeout, polling the pipe thread every 10ms.

To handle the exception mentioned in #19 I added a try/except around the stdin flush call, for possible exceptions when the daemon is dead. If it is, the terminate_daemon method is called, clearing the global variable, and the command payload is resent to a newly spawned daemon.

Finally, I made the prints optional via a DEBUG flag, replaced all the double quotes with single quotes to uniformize them (missed that in #21) and added a "terminate daemon" command, so that the users can restart the daemon if they wish.

Since the commands are now async, the user has no feedback as to what happens, so I added a status message on the bottom status bar while the daemon is waited upon.

Open Questions and Pre-Merge TODOs

[x] Windows testing.
[x] OSX testing.
[x] I think there's a possible bug in the async timeout, as it's not canceled if the daemon is terminated.
[x] Bug: the first command output is not consumed and the plugin is always one command behind as a result.
[x] ~Bug: the plugin will replace everything in the active view when a command finishes, but the active view might not be the one the command was called in.~

Also, it would have been nice to use async/await, but they are only available starting from Python 3.5, and Sublime is using 3.3, AFAIK.

monovertex commented 6 years ago

@trotzig The only things I want to address are the 2 TODOs in the PR description. I'll test it on Windows tomorrow as well and handle that async timeout cancel (which is probably a really small change).

monovertex commented 6 years ago

OK, I handled that possible bug as well. I just want to test that this version works correctly on Windows and it's good to go from my side.

monovertex commented 6 years ago

@trotzig I tested it on Windows and it's OK. However, I used the plugin in this version more extensively yesterday and there are two bugs still exhibiting:

The plugin will not properly consume the response of the first command run, which starts the daemon, and it will always be one command behind the user's actions.
I think, since this is now async, the user can switch views after the command is called, but before it finishes executing, and the plugin will happily replace everything in the current active view, not in the view from where the command was called.

I would prefer to fix these two bugs as well before this is merged.

trotzig commented 6 years ago

Ok, cool. Those sound like blockers to me. I'll do some testing on my side anyway just to make sure there aren't any more blockers.

trotzig commented 6 years ago

Things look good from my end. The first bug is definitely something we need to fix before we ship. The second bugfix might be possible to defer until we have someone report it (or you encounter it while you use the tool). It might not be an actual problem, although I'll let you make the decision about whether to fix it or not.

monovertex commented 6 years ago

@trotzig I fixed the first bug. It was caused by the piece of code that consumes the first line coming out of the daemon pipe when the process is started. I moved the handling of the WAITING_FOR_DAEMON_RESPONSE lock so that it will be done when the command is started / finished, instead of when the daemon read is started / finished.

The second bug is a non-issue, from my testing I figured out that self.view will always be the same for a command instance and is not updated when the users switches the view.

~So right now it's all good from my point. I'd appreciate it if you could double check as well.~

Unfortunately it still seems like there are some race conditions in the current code. I'll keep testing it and return with a fix.

trotzig commented 6 years ago

I appreciate the commitment here. Switching from sync to async was bound to expose race conditions, and I'm glad you're catching them early on.

monovertex commented 6 years ago

@trotzig I fixed the remaining bugs. I had to split the file to extract the daemon code into a separate class, so the diff is larger now, but I think the code is better and clearer this way.

Please test the plugin a bit if you can and let me know if you find anything else.

trotzig commented 6 years ago

I’ll give it a spin first thing on Monday! First I’ve got to enjoy this afk weekend.

monovertex commented 6 years ago

No rush, I'll be away the entire next week anyway. Enjoy the weekend!

trotzig commented 6 years ago

Works great! Thanks for a great contribution! I'll make a release right away.

janpaul123 commented 6 years ago

My import-js now keeps hanging on "Running ImportJS command..." with no error messages in the console. I'm unsure on how to debug this any further.

janpaul123 commented 6 years ago

Hm, downgrading to 3.0.0 fixed the issue. 😰 Created an issue here https://github.com/Galooshi/sublime-import-js/issues/23

Galooshi / sublime-import-js