search

stefanofornari / BshConsole

A BeanShell console with history, vars/commands completion and more...
Apache License 2.0
2 stars 1 forks source link

Yet another approach to interrupt #15

Open nickl- opened 6 years ago

nickl- commented 6 years ago

See if you like this better.

I wrapped the Interpreter in a singleton BshSession which has start, restart and close methods. It implements ConsoleInterface which just seems cleaner too.

I made the getBshPrompt command into a static class, see what you think, this might make things easier as well.

It interrupts multilines without EOF errors and can still interrupt and recover from loops.

All upto you.

stefanofornari commented 6 years ago

@nickl- sounds like a much cleaner approach. I cannot compile though:

ste/beanshell/BshSession.java:[40,14] error: no suitable constructor found for Interpreter(BshSession,NameSpace,Interpreter)

do I need to use any specific beanshell branch?

nickl- commented 6 years ago

you need to update your beanshell

just pull the latest

stefanofornari commented 6 years ago

This is what I have done. I'll try again from scratch.

stefanofornari commented 6 years ago

hey @nickl- , I manage to compile. the issue was in the test classes that have not been changed accordingly... for now it is ok, but for bigger PRs it may be an issue...

Anyway, I did some tests, see the videos below:

https://zefiro.me/share/WVLaQZxjMRCLjYKy https://zefiro.me/share/Kbw8joWyS5hWYUj1 https://zefiro.me/share/BbwtrDKSFUPyPH15

I see a few problems:

One note: because of the way the console and the interpreter read the streams from different threads, you have to follow a proper sequence to close cleanly the streams. This is the sequence I use:

 private void reset() {
        println("(...)");

        try {
            PipedWriter newPipe = new PipedWriter(),
                    oldPipe = pipe;
            this.in = new PipedReader(newPipe);
            parser = new Parser(in);
            pipe = newPipe;
            discard = true;
            oldPipe.close();
        } catch (IOException x) {
            // nothing to do...
            x.printStackTrace();
        }
}

In essence, before closing the pipe we have to prepare all other streams. Once I call oldPipe.close(), Line (or isEOF) exits unblocking the Interpreter thread. This is a trick just to get things synchronized.

nickl- commented 6 years ago

Can you please show examples of the problems you are listing. It would help if you can give steps to reproduce or at least provide the output you are getting. I am not getting any of that, I've tried.

As mentioned before, there are still issues with the test dependencies, I cannot compile the tests.

stefanofornari commented 6 years ago

have you manage to see the videos? those are the steps to reproduce and notes. it may have to do with differences in JVM or OS. with multi threading there are always different behaviours. I mostly play with print("+\n, fun() { +\n, class A {.... to me it happens quite systematically.... On the other hand does my BshConsole work fine on your terminal?

there are a bunch of dependencies that need to be reconstructed from the git repositories... I know it is not handy, I have to work on it, so for now do not bother.

Ste

On Sat, Jul 7, 2018 at 3:11 PM, Nick Lombard notifications@github.com wrote:

Can you please show examples of the problems you are listing. It would help if you can give steps to reproduce or at least provide the output you are getting. I am not getting any of that, I've tried.

As mentioned before, there are still issues with the test dependencies, I cannot compile the tests.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/stefanofornari/BshConsole/pull/15#issuecomment-403214724, or mute the thread https://github.com/notifications/unsubscribe-auth/AACogqDM17klv1zUEKPySlmQtpIB_5Z_ks5uELN_gaJpZM4VFniN .

nickl- commented 6 years ago

Oh sorry I didn't see the links... checking again.

stefanofornari commented 6 years ago

Oh, I forgot one comment: I learned to avoid as much as I can static fields as they become a nightmare while writing tests.

nickl- commented 6 years ago

Made some fixes to interpreter see beanshell/beanshell@ba34692

Problems reported.

I cannot reproduce this, but were able to make it worse. iow Make it happen on iTerm. I will try and test this on ubuntu and see if I can find out why it is happening.

Turns out in some cases the in.close() is blocking and we never get to complete the restart in time. To avoid this being a problem I'm calling Interpreter.close() as a Runnable. This also improves the restart process as we can continue instantiating the new interpreter with less delay.

These should all be avoided now, I added a way to configure a sleep delay for the interpreter to wait for close which seems to do the trick. Also avoid printing results by setting setShowResults to false. Please see if it now solves your issues I did test for all the edge cases from your screen captures and they are now resolved here.

You must show me how you do the video shares, it is much better than google drive for sure.

nickl- commented 6 years ago

Oh, I forgot one comment: I learned to avoid as much as I can static fields as they become a nightmare while writing tests.

You must show me where you are having issues. Static fields are useful in the right context, for example constants in java are of course static final and in the case of BshSession we have a static field as the instance reference, which is how we produce the singleton pattern. I think the session works well as a singleton, there can only ever be 1 instance of the session which we access from BshSession.init(). Allowing the session to manage the bsh instance in use as the instance variable bsh. I don't know of another way to make a singleton in java, do you?

Static variables can be an issue for multi threading but of course constants can never change so this does not pose the same problems.

I wouldn't simply avoid them, they serve a fundamental role in java. Just need to be used correctly.

stefanofornari commented 6 years ago

commenting is quite difficult here ... :)

Made some fixes to interpreter see beanshell/beanshell@ba34692

Problems reported.

issues with the prompt not showing at startup

I cannot reproduce this, but were able to make it worse. iow Make it happen on iTerm. I will try and test this on ubuntu and see if I can find out why it is happening.

the pipe/input stream getting lost in some circumstances

Turns out in some cases the in.close() is blocking and we never get to complete the restart in time. To avoid this being a problem I'm calling Interpreter.close() as a Runnable. This also improves the restart process as we can continue instantiating the new interpreter with less delay.

lexical errors thrown in some cases

These should all be avoided now, I added a way to configure a sleep delay for the interpreter to wait for close which seems to do the trick. Also avoid printing results by setting setShowResults to false. Please see if it now solves your issues I did test for all the edge cases from your screen captures and they are now resolved here.

You must show me how you do the video shares, it is much better than google drive for sure.

mmh... but why are we looking for something else than what is working quite nicely? if we converge with https://github.com/beanshell/beanshell/pull/86, BshConsoleInterpreter becomes quite clean.

You must show me where you are having issues. Static fields are useful in the right context, for example constants in java are of course static final and in the case of BshSession we have a static field as the instance reference, which is how we produce the singleton pattern. I think the session works well as a singleton, there can only ever be 1 instance of the session which we access from BshSession.init(). Allowing the session to manage the bsh instance in use as the instance variable bsh. I don't know of another way to make a singleton in java, do you?

Static variables can be an issue for multi threading but of course constants can never change so this does not pose the same problems.

I wouldn't simply avoid them, they serve a fundamental role in java. Just need to be used correctly

In general: indeed constants are good as static, in addition to few other cases. My rule is: if you do not absolutely need it, avoid it. This is also the case of singleton. The cases were you really need a strongly enforced singleton are so rare that I do not care. 99% of the cases an instance field is enough. I give you a simple example why they are troublesome. I run into it writing a test method in InterpreterTest. Look at this https://github.com/stefanofornari/beanshell/commit/9056926ee29fceba9afd66580b4b03a7fd38dffb. can you explain to me why if I add (I did it by mistake) bsh.setShutdownOnExit(false); in a test, check_system_object() fails? you could spend hours before figuring it out. And this is a simple example. But because JUnit runs tests in parallel with static fields you set yourself for nights spent to avoid not deterministic issues in running your test...

stefanofornari commented 6 years ago

You must show me how you do the video shares, it is much better than google drive for sure.

It is the product we develop :) go to http://zefiro.me and give it a try :)

nickl- commented 6 years ago

commenting is quite difficult here ... :)

The trick is to put everything on one line, then you need one > at the start to quote the whole paragraph.

nickl- commented 6 years ago

But because JUnit runs tests in parallel with static fields you set yourself for nights spent to avoid not deterministic issues in running your test...

All good experience... keep doing that instead of avoiding it, you'll be a master of static in no time.

Check out ThreadLocal its magic...

nickl- commented 6 years ago

mmh... but why are we looking for something else than what is working quite nicely? if we converge with beanshell/beanshell#86, BshConsoleInterpreter becomes quite clean.

If it was working then why am I wasting my time here? You had a list of issues, they have now been resolved, please test and let me know.

I would also like to know if the prompt on startup issue is now resolved on ubutu, I haven't had the chance to test on ubuntu yet. Please let me know...

nickl- commented 6 years ago

It is the product we develop :) go to http://zefiro.me and give it a try :)

what format should the videos be in?

nickl- commented 6 years ago

It is the product we develop :) go to http://zefiro.me and give it a try :)

Is this related to funambol? I've been using them for contacts since they started. You working for funambol?

=( would've beet mewl if my cellphone username worked.

stefanofornari commented 6 years ago

:) indeed it is Funambol, and indeed I work in Funambol...

Ste

On Sun, Jul 8, 2018 at 5:38 PM, Nick Lombard notifications@github.com wrote:

It is the product we develop :) go to http://zefiro.me and give it a try :)

Is this related to funambol? I've been using them for contacts since they started. You working for funambol?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/stefanofornari/BshConsole/pull/15#issuecomment-403296105, or mute the thread https://github.com/notifications/unsubscribe-auth/AACogjIHjgCHjYpSGtqDSMsoTRN7N01zks5uEieNgaJpZM4VFniN .