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

WIP: Support asynchronous operations (promises) #81

Closed
wants to merge 4 commits into from

Conversation

tlwr
Copy link
Contributor

@tlwr tlwr commented Feb 14, 2017

Summary

More details in #79

I've managed so far to keep it as backwards compatible as possible, there are no configuration changes needed for any operations.

Functionality example

An operation could only look like this before:

myOp: function(input, args) {
  var a = doSomething();
  if (a.err) {
    throw a.err;
  }
  return doSomethingElse(a);
}

But if doSomething was asynchronous then we would not be able to use it...

Now we can also do:

myAsyncOp: function(input, args) {
  return new Promise(function(resolve, reject) {
    doAsyncSomething()
    .then(function(a) {
      resolve(doSomethingElse(a));
    })
    .catch(function(err) {
      reject(err)
    });
  };
}

Operational example

I have added an operation called "Wait", this is what it looks like when an operation is in progress:
image

I'd love your feedback on how I can improve this, or if you find any bugs (and because it is quite a large functionality change I do expect there to be some fiddly edge-cases).

tlwr added 3 commits February 14, 2017 14:54
Operations can now:
1) return their progress directly.
2) throw an error.
3) (ADDED) return a promise:
  + that resolves to their progress.
  + that rejects an error message (like throwing but asynchronous).
  For an example see the new operation "Wait" (Flow Control)

Added a flow control operation "Wait", which waits for the number of
milliseconds passed in as its argument. It is a fairly useless operation
but it does demonstrate how asynchronous operations now work.

A recipe like:
```
Fork
Wait (1000ms)
```
will only wait for 1000ms (each wait runs at the same time as each
other).

I have not looked into performance implications yet, also this code is
probably more complicated than it needs to be (would love help on this).
@n1474335
Copy link
Member

This is brilliant work and something that I've wanted to get sorted out for a while. When I've got a bit of spare time I'll go through it in detail and try to find edge cases where it causes problems, but for now just wanted to give a big thumbs up. We'll definitely get this merged once we've had a chance to test it thoroughly.

If fork was called with no following operations, progress was undefined,
this caused an infinite loop.
@tlwr
Copy link
Contributor Author

tlwr commented Feb 22, 2017

Because this is a big change it would seem like a good time to add some functionality for testing operations. To keep with the very simple and lightweight structure of CyberChef it would be nice to not have to pull in tools like Selenium and PhantomJS just to test operations.

I hacked together a quick implementation to help test the above changes and I think it makes sense to actually have this functionality permanently (and for all operations).

My very simple implementation.

  1. Run grunt test
  2. Open browser to the test build
  3. The tests are run and the user can see which tests are passing/failing/erroring.

The actual CyberChef code doesn't have to be modified to enable this functionality because of the separation of the actual HTMLApp and Chef/Recipe/etc.

  • → means the test is still running
  • ✓ means the test is passing.
  • ! means the test is failing (The output and the expectedOutput differ).
  • ❌ means the test is erroring when it should not (expectedError is false or undefined).

screenshot from 2017-02-22 15-42-48

Tests are currently defined (all together) in a list, each test is defined like so:

    {   
        name:"Fork & Merge nothing",
        input: "", 
        expectedOutput: "", 
        expectedError: false,
        recipeConfig: [
            {   
                op: "Fork",
                args: ["\n", "\n", false],
            },  
            {   
                op: "Merge",
                args: [], 
            },  
        ]   
    }

The recipeConfig is as it is elsewhere in the codebase (and the URL).

The expectedOutput is what it will test the actual output against.

The expectedError (optional) is for cases when we should expect an error (e.g. "To base" with a non-number).


Should I commit it (after cleaning it the code a bit), if so, should I put it here or in another PR?

@n1474335
Copy link
Member

Another thing I can take off my to do list. Yes, definitely commit this. I'd put it in a separate pull request as it's not strictly linked to this one.

@tlwr tlwr mentioned this pull request Feb 23, 2017
@tlwr
Copy link
Contributor Author

tlwr commented Feb 23, 2017

Glad to lighten the load; moved this into it's own PR #84.

@n1474335
Copy link
Member

I have made a few largely aesthetic changes and created a new branch, feature-async-ops to work on this longer term. There are a few things which prevent us from merging it into master just at the moment, including:

  • There are some bugs surrounding some of the Flow Control operations, particularly 'Conditional Jump'. I want to make sure this is fully tested before we roll it out and that will be a lot easier once the test runner has been merged, so we'll get that sorted out first.
  • Using promises means that we'll loose support for a lot of browsers. CyberChef is fairly forward leaning so I'm not massively concerned about support for very old browsers, but this is a pretty drastic step up. We should think about how to improve backwards compatibility (perhaps by using something like Babel) before we go ahead and merge this into the production version.
  • It would be nice to make better use of this functionality, perhaps by adding the ability to cancel bakes if they are taking too long (at least those that are asynchronous). This could require quite a bit more work and I think we should thrash that out in the new branch and then perhaps up the major version number when we eventually merge it into master.

For now I will close this PR - all new work should be merged into feature-async-ops. Thanks very much for getting the ball rolling, this is very promising.

@n1474335 n1474335 closed this Feb 28, 2017
BRAVO68WEB pushed a commit to BRAVO68WEB/CyberChef that referenced this pull request May 29, 2022
…fdd784c06a7012a0

[Snyk] Upgrade vue-material-tabs from 0.0.7 to 0.1.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants