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

API base url parsing does not work for eventstream #4521

Closed
nazarhussain opened this issue Sep 8, 2022 · 2 comments · Fixed by #6179
Closed

API base url parsing does not work for eventstream #4521

nazarhussain opened this issue Sep 8, 2022 · 2 comments · Fixed by #6179
Assignees

Comments

@nazarhussain
Copy link
Contributor

Describe the bug

If someone using a base url ending with / that make the api.events. namespace to not work. All other namespaces works fine.

Expected behavior

It should work fine without any error by parsing the base url correctly. If not then it should throw some error to users to tell them base url format is not valid.

Steps to Reproduce

import {getClient} from "@lodestar/api";

const api =  getClient({baseUrl: "http://127.0.0.1:5001/"});

Desktop (please complete the following information):

  • Branch: [unstable]
  • Commit hash: [e8232]
@philknows
Copy link
Member

Make sure it's not an issue after #5128

@nflaig
Copy link
Member

nflaig commented Dec 12, 2023

I have opened a PR to address this #6179 but I am not sure if we should throw an error if server responds with a 404 (which it did previously due to // in URL).

Right now the only errors that are considered "unrecoverable" are 400 and 500 status codes, those are also defined in the spec.

// Consider 400 and 500 status errors unrecoverable, close the eventsource
if (errEs.status === 400) {
reject(Error(`400 Invalid topics: ${errEs.message}`));
}
if (errEs.status === 500) {
reject(Error(`500 Internal Server Error: ${errEs.message}`));
}

The problem is that the eventstream will be closed permanently if we reject on error here which might not be desired for 404 errors because the issue could be related to a proxy misconfiguration, same goes for 401 errors, those could be potentially recoverable and it might be safer to retry connecting the eventstream.

Even if we don't consider 401 / 404 as fatal errors we should definitely find a solution to log those errors which is currently not done as handler does not have access to logger

// TODO: else log the error somewhere
// console.log("eventstream client error", errEs);

If not then it should throw some error to users to tell them base url format is not valid.

The http client throws an error if base url format is invalid, the expected behavior stated in this PR should be addressed once #6179 is merged

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.

4 participants