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.

Sign up for GitHub

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

Jump to bottom

Notifications on a websocket transport trigger a timeout after 5000ms #231

Closed
eniv opened this issue Dec 14, 2020 · 1 comment · Fixed by #232
Closed

Notifications on a websocket transport trigger a timeout after 5000ms #231

eniv opened this issue Dec 14, 2020 · 1 comment · Fixed by #232
Assignees
@zcstarr
Labels
bug Something isn't working released
Projects
OpenRPC

Comments

@eniv
Copy link

eniv commented Dec 14, 2020

Describe the bug
While the notification is formatted and sent to the web-socket as expected , it triggers a timeout after 5000ms since a response is expected.

To Reproduce

const transport = new WebSocketTransport(`ws://${server_ip}:443/rpc`);
const requestManager = new RequestManager([transport]);
const client = new Client(requestManager);
client.notify({method: "foo", params: {"bar": "foobar"}});

Expected behavior
No response should be expected for notifications.

Additional context
WebSocketTransport.sendData has the following signature:

public async sendData(data: JSONRPCRequestData, timeout: number | undefined = 5000): Promise<any>

which defaults to a 5000ms timeout
@zcstarr zcstarr self-assigned this Dec 16, 2020
zcstarr added a commit that referenced this issue Dec 16, 2020
@zcstarr
fix: rm notification default timeout
d47e58a 
fixes #231
@zcstarr zcstarr mentioned this issue Dec 16, 2020
Merged
zcstarr added a commit that referenced this issue Dec 16, 2020
@zcstarr
fix: this corrects default timeout to be disabled by specifing null.
accbf80 
This fix was needed because the old default used undefined, which
in turn triggered the default timeout when left unspecified, but
would not allow the user to disable timeouts. This refactor allows
users to be specific about what they want specifying default(undefined),custom
timeout, or no timeout (null).

fixes #231
zcstarr added a commit that referenced this issue Dec 16, 2020
@zcstarr
fix: this corrects default timeout to be disabled by specifing null.
c79d213 
This fix was needed because the old default used undefined, which
in turn triggered the default timeout when left unspecified, but
would not allow the user to disable timeouts. This refactor allows
users to be specific about what they want specifying default(undefined),custom
timeout, or no timeout (null).

fixes #231
@shanejonas shanejonas added the bug Something isn't working label Dec 17, 2020
@shanejonas shanejonas added this to To do in OpenRPC via automation Dec 17, 2020
OpenRPC automation moved this from To do to Done Dec 17, 2020
openrpc-bastion added a commit that referenced this issue Dec 17, 2020
@openrpc-bastion
chore(release): 1.6.2 [skip ci]
e30d0c0 
## [1.6.2](1.6.1...1.6.2) (2020-12-17)

### Bug Fixes

* **RequestManager:** ignore missing id ([6bc8116](6bc8116))
* jsonrpc error instanceof ([c85f501](c85f501)), closes [#209](#209)
* this corrects default timeout to be disabled by specifing null. ([c79d213](c79d213)), closes [#231](#231)
@openrpc-bastion
Copy link
Member

🎉 This issue has been resolved in version 1.6.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@rowend36 rowend36 mentioned this issue May 6, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zcstarr zcstarr

Labels
bug Something isn't working released
Projects
OpenRPC
  
Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: rm notification default timeout open-rpc/client-js
4 participants
@zcstarr @shanejonas @eniv @openrpc-bastion