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

Fix 954 #956

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix 954 #956

wants to merge 4 commits into from

Conversation

yumauri
Copy link

@yumauri yumauri commented Dec 3, 2024

PoC to fix #954

@yumauri
Copy link
Author

yumauri commented Dec 3, 2024

I will add few more tests tomorrow

@fabian-hiller
Copy link
Owner

Thank you for creating this PR. There is a problem. The current implementations ensure consistency. The same dataset will always return the same result, regardless of whether the promises take different amounts of time. But if we abort after the first input validation with an issue, the order may be random. Of course, we could choose performance over consistency in this case, but it would be a breaking change.

Your implementation has added a lot of extra code. Let us try to keep it to a minimum. Here is a quick idea I had in mind. Feel free to experiment with it and share your thoughts.

const valueDatasets = await Promise.all(
let shouldAbort = false;
const valueDatasets = await Promise.all(
  Object.entries(this.entries).map(async ([key, schema]) => {
    if (!shouldAbort) {
      const value = input[key as keyof typeof input];
      const valueDataset = await schema['~run']({ value }, config);
      if (config.abortEarly && valueDataset.issues) {
        shouldAbort = true;
      }
      return [key, value, valueDataset] as const;
    }
  })
);

@fabian-hiller fabian-hiller self-assigned this Dec 4, 2024
@fabian-hiller fabian-hiller added the enhancement New feature or request label Dec 4, 2024
@yumauri
Copy link
Author

yumauri commented Dec 4, 2024

This will not work because, once launched, a promise can’t be aborted from the outside. If there are many checkAsync actions in the schema, all of them will be launched synchronously, and the if (!shouldAbort) check will pass for all of them. After launch, you no longer have control over the await it will wait until the promise is resolved.

This can be mitigated by wrapping the ~run call in some kind of “abortable promise” wrapper, which uses Promise.race under the hood and can be aborted externally. This will also require passing the AbortController.signal everywhere to signal other running promises (though I wanted to try that in my approach as well).

But I doubt this will significantly reduce the amount of code…

But if we abort after the first input validation with an issue, the order may be random

As with any side-effect check combined with abortEarly ¯\_(ツ)_/¯
https://valibot.dev/playground/?code=JYWwDg9gTgLgBAKjgQwM5wG5wGZQiOAcg2QBtgAjCGQgbgCh6BjCAO1XgGUmALAUxDI4AXkwA6CBQBWfJjAAUAb3pw4A5MFIAucWGBg+8jGI5RgrAObyAlABpxvWQGt5NkQD44AWWQweYqGRWABN8N08ABjEAVms7FTgwNFQAd2hgnWM9AyMTGDNLG3tjRyYXN2FPHz8AoNCQcLgo2PiAX2sGZjYOOCg+VABXUnhRY1RkbD4ABWQoVENufkF7ZVV1TR1CKSC+AAE+AA9kcFI+MRYQQlsEpNRU9M2ARgAmAGYAFmiANgB2AA4rvRWisUFRYABRWakACeOnyAz4cHanRY7AgpzEpAgVj6g2GHXoQA

I don’t think this is a breaking change. But even if you think it is, the library is still in its zero version, so it’s not a big deal, in my opinion.

@fabian-hiller
Copy link
Owner

This will not work because, once launched, a promise can’t be aborted from the outside.

That may be true, but we can still somehow change what the promise actually does, and e.g. not execute the actual expensive logic, or am I wrong?

If I understand your current code correctly it executes the promises in sequential order and I am not sure if this is an option.

But even if you think it is, the library is still in its zero version, so it’s not a big deal, in my opinion.

I agree, and am willing to investigate this case and favor performance if we find a good and uniform solution that works for any async schema.

@yumauri
Copy link
Author

yumauri commented Dec 5, 2024

but we can still somehow change what the promise actually does

No, a promise is like a black box. Once launched, you can only wait for it to resolve (or reject). Or ignore it.
The pattern of wrapping a promise in Promise.race to "abort" it doesn’t actually abort the promise itself; it just ignores it.

Or maybe I just don’t understand what you’re pointing out 🤔

If I understand your current code correctly it executes the promises in sequential order and I am not sure if this is an option.

No, the idea was that I collect promises in an array but do not await them. Later, using Promise.race on that array, I get the winning promise/validation, check if it has issues or not, and if it doesn’t, I remove it from the array and then await the next winner, and so on. However, all asynchronous validations are launched simultaneously.

But after I added a few tests, I found a critical issue in my approach: after removing a promise from the array, the indexes of the remaining promises became misaligned.

So, I rewrote the PoC without using Promise.race at all :)
I also added a test that clearly demonstrates that all async validations start in parallel.

Also, I added logic so that if I encounter synchronous issues while executing validations, it stops right there and doesn’t launch the async validation at all (if it is located below the synchronous one).

As for the rest, what I want to try is to add the possibility to abort running promises by passing an AbortController.signal to each one of them. This way, the user can hook into this signal to abort an HTTP request, for example, if validation fails on another field.

@fabian-hiller
Copy link
Owner

Or maybe I just don’t understand what you’re pointing out 🤔

Look at this simple example. .foo is executed before .bar and can therefore abort the expensive await timeout execution. This probably only solves the problem of this PR if sync validation is mixed with async validation.

Thank you for your research! I really appreciate your efforts and hope we can improve the current implementation. I don't have much time at the moment, but I will try to have a closer look at the end of December or in January. Feel free to share any research you do here, as well as alternative implementations if you find any. I can also share this PR on social media to get additional feedback and ideas from others.

@yumauri
Copy link
Author

yumauri commented Dec 7, 2024

Yeah, this is almost the same as what I’m doing here — if the validation result is data (not a promise-like object) and has issues, it stops the iteration right there and doesn’t even launch the following promises (already launched ones will be ignored). And yes, this will fail a test with two asynchronous operations of different durations.

I can also share this PR on social media to get additional feedback and ideas from others.

Sure, why not 👍

@yumauri
Copy link
Author

yumauri commented Dec 7, 2024

I was thinking about how I could add an AbortController here to signal other running promises about an aborted validation, so they can handle it correctly (for example, aborting a pending HTTP request). In doing so, I stumbled upon a question that hadn't occurred to me before: why are there no exception handling mechanisms in schemas or actions? If I throw an exception from a check action or custom schema, it will break parsing, even with safeParse (which doesn't seem very "safe" to me).

I'm raising this in the following context: if you add an AbortController.signal to a fetch call and then call abort on the controller, fetch will be rejected with an AbortError and will throw an exception inside the await. So, users must manually handle this case by catching possible errors and returning false. Why not wrap any validation in a try-catch block and treat exceptions as false?

Maybe I'm shooting from the hip here, as the library's API has been extensively discussed, and it seems no one is concerned about this issue — I haven't found any related discussions or issues. I also haven't found any reasoning behind this design decision.

@yumauri
Copy link
Author

yumauri commented Dec 8, 2024

On the other hand, maybe this is not a problem in my proposal, because after validation abortion all pending promises results are ignored anyways 🤔 I will investigate this.

But I'm still curious about question above in general.

@fabian-hiller
Copy link
Owner

So far, we expect users to catch errors and return true or false. This makes our code consistent throughout the library. We could add a try/catch block to check and checkAsync, but this would unnecessarily increase the bundle size when not needed, and raises the question of whether other actions with custom code should also be wrapped in try/catch. I am open to discuss this as there are pros for both sides.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Looks like abortEarly doesn't abort async parsing
2 participants