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

Cannot change the default value of the batch request timeout #6190

Closed
Muhammad-Altabba opened this issue Jun 12, 2023 · 14 comments · Fixed by #6336
Closed

Cannot change the default value of the batch request timeout #6190

Muhammad-Altabba opened this issue Jun 12, 2023 · 14 comments · Fixed by #6336
Labels
4.x 4.0 related Discussion Good First Issue Great to learn the internals of web3.js

Comments

@Muhammad-Altabba
Copy link
Contributor

The constant existed at
https://github.com/ChainSafe/web3.js/blob/ab8013101665d687cb140fee2c1f528b5eec562b/packages/web3-core/src/web3_batch_request.ts#L23:

export const DEFAULT_BATCH_REQUEST_TIMEOUT = 1000;

Is used without the possibility to change it:

	public async execute(): Promise<JsonRpcBatchResponse<unknown, unknown>> {
		if (this.requests.length === 0) {
			return Promise.resolve([]);
		}

		const request = new Web3DeferredPromise<JsonRpcBatchResponse<unknown, unknown>>({
			timeout: DEFAULT_BATCH_REQUEST_TIMEOUT,
			eagerStart: true,
			timeoutMessage: 'Batch request timeout',
		});

                ...

Environment

web3.js version 4.x

@Muhammad-Altabba Muhammad-Altabba added Discussion Good First Issue Great to learn the internals of web3.js 4.x 4.0 related labels Jun 12, 2023
@sleepyqadir
Copy link

@Muhammad-Altabba can you assign it to me?

@jdevcs
Copy link
Contributor

jdevcs commented Jun 19, 2023

@sleepyqadir

@truongezgg
Copy link

@sleepyqadir anything new sir?

@shubhamshd
Copy link
Contributor

Hi @sleepyqadir any update to the issue ?
are you still working on this?

@shubhamshd
Copy link
Contributor

Hi @Muhammad-Altabba
I am not sure if assignee is actively working on the issue, but I would like to contribute by implementing the suggested changes with your approval

I kindly request your permission to proceed with this issue and contribute to the project's improvement. If there are any specific instructions or considerations I should be aware of, please let me know.

Thank you for your time and consideration. I look forward to your response.

@newTomas
Copy link

Adding this option isn't difficult, but how the hell do you build this project? I even get errors on yarn install and I don’t intend to go through 14 circles of hell (checklist) for the sake of this garbage. Who needs my solution - here is https://github.com/newTomas/web3.js/tree/patch-1
Personally, I find it easier to put up with a timeout of 1 second.

@newTomas
Copy link

@shubhamshd
If you want to do this issue you can fork the project, make edits and create a pull request with a link to this issue, and if the pull request is accepted, the issue will close automatically
I don't think anyone is doing this now.
https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

@Muhammad-Altabba
Copy link
Contributor Author

Hi @shubhamshd ,
Sorry for the late reply. Yes, you can please work on it. As @sleepyqadir did not seem to take action. And no one from the team is working on it as there are other tasks with higher priority at the moment.

@shubhamshd
Copy link
Contributor

Thanks @newTomas @Muhammad-Altabba will work on it this weekend

@shubhamshd
Copy link
Contributor

Hi @Muhammad-Altabba @newTomas
I was working on the changes for this issue, and have little bit of confusion with regards to approach for the solution.

I was planning to introduce a private property in the Web3BatchRequest class and initiate it via constructor, something similar as follows :

` private _defaultBatchRequestTimeout: number; // Add this line

public constructor(
requestManager: Web3RequestManager,
defaultBatchRequestTimeout = 1000 // Add default value for the timeout
) {
this._requestManager = requestManager;
this._requests = new Map();
this._defaultBatchRequestTimeout = defaultBatchRequestTimeout; // Assign the provided or default value
}
`

But that would need subsequent changes at all places where this Web3BatchRequest object is called/initialized right?

is this something fine or am I making it complex?

@newTomas
Copy link

newTomas commented Aug 6, 2023

@shubhamshd
Copy link
Contributor

Hi @Muhammad-Altabba @newTomas It took some time to understand the web3batchrequest class and its usage, and definitely the solution suggested by @newTomas looks promising to me!!!

I have opened a pr with the required change, please review and let me know, if any changes required!!!

thanks both :)

@NongChen1223
Copy link

Is the problem still unresolved? I don't see this code in the source code, my version is "^4.6.0"

@jdevcs
Copy link
Contributor

jdevcs commented Apr 15, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x 4.0 related Discussion Good First Issue Great to learn the internals of web3.js
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants