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

WebSocket connection closed without (our custom) closing frame in case of a timeout #1914

Closed
mpass99 opened this issue Sep 18, 2023 · 5 comments · Fixed by #1916
Closed

WebSocket connection closed without (our custom) closing frame in case of a timeout #1914

mpass99 opened this issue Sep 18, 2023 · 5 comments · Fixed by #1916
Assignees

Comments

@mpass99
Copy link
Contributor

mpass99 commented Sep 18, 2023

See CODEOCEAN-TW


After running into a runner or execution timeout we should send the timeout message to CodeOcean. This behavior is tested by the unit tests TestExitHasTimeoutErrorIfRunnerTimesOut and TestReturnsAfterTimeout, and the e2e test TestCommandReturnsAfterTimeout.


CodeOcean shows the wrong error Alle Ausführungsumgebungen sind momentan in Benutzung. Probiere es später nochmal.

@mpass99
Copy link
Contributor Author

mpass99 commented Sep 18, 2023

By changing the debug level of CodeOcean to debug, I was able to verify that CodeOcean receives the timeout message.

Image

I tried to debug the behavior of CodeOcean, but unfortunately I was not even able to get added debug statements in connection.rb to output ☹️

@MrSerth
Copy link
Member

MrSerth commented Sep 18, 2023

Awesome, thanks for having a further look there! I thought I had a look there, but obviously I didn't.

When investigating now, I identified that the JSON response sent by Poseidon is not recognized as valid and hence not accepted:

BACKEND_OUTPUT_SCHEMA.valid?({"type": "timeout"})
=> false

BACKEND_OUTPUT_SCHEMA.valid?({"type": "start"})
=> false
BACKEND_OUTPUT_SCHEMA.valid?({"type": "exit", data: 0})
=> true

BACKEND_OUTPUT_SCHEMA.valid?({"type": "stdout", data: "test"})
=> true

BACKEND_OUTPUT_SCHEMA.valid?({"type": "stderr", data: "test"})
=> true

BACKEND_OUTPUT_SCHEMA.valid?({"type": "error", data: "test"})
=> true

The test is performed here with this schema loaded from here. The library JSONSchemer we use got recently upgraded.

Can you tell whether the schema and output should validate to each other or whether we see a bug in the library?

@MrSerth
Copy link
Member

MrSerth commented Sep 19, 2023

Indeed, I was able to identify that the recent JSONSchemer upgrade is responsible for the bug. Hence, I downgraded JSONSchemer with f6975ac and added some Sentry logging with 56b6380 to prevent further (unnoticed) issues.

I think we should proceed with testing further (is our schema valid...) and otherwise report this as a bug with a minimal reproduction example upstream. The following command might be handy for testing:

JSONSchemer.schema(JSON.parse(File.read('lib/runner/backend-output.schema.json'))).valid?({"type" => "timeout"})

It works with JSONSchemer 1.0.3 and fails with 2.0.0.

@mpass99 Would you mind assisting me from here?

@mpass99
Copy link
Contributor Author

mpass99 commented Sep 19, 2023

The Online JSON Schema Validator reports that our schema is invalid. The required attribute should not be at the specific property but one more outer as an array containing the required properties (such as described in the JSON schema).

By correcting the JSON schema (see openHPI/poseidon#456), the JSONSchemer accepts now all listed events.

# all true
puts BACKEND_OUTPUT_SCHEMA.valid?({"type": "timeout"})
puts BACKEND_OUTPUT_SCHEMA.valid?({"type": "start"})
puts BACKEND_OUTPUT_SCHEMA.valid?({"type": "exit", data: 0})
puts BACKEND_OUTPUT_SCHEMA.valid?({"type": "stdout", data: "test"})
puts BACKEND_OUTPUT_SCHEMA.valid?({"type": "stderr", data: "test"})
puts BACKEND_OUTPUT_SCHEMA.valid?({"type": "error", data: "test"})

@MrSerth
Copy link
Member

MrSerth commented Sep 19, 2023

By correcting the JSON schema (see openHPI/poseidon#456), the JSONSchemer accepts now all listed events.

Indeed, I was able to confirm this. Let's create a PR for CodeOcean as well 👍.

mpass99 added a commit that referenced this issue Sep 19, 2023
that included a wrong usage of the `required` attribute.

See #1914 and openHPI/poseidon#456.
MrSerth pushed a commit that referenced this issue Sep 19, 2023
that included a wrong usage of the `required` attribute.

See #1914 and openHPI/poseidon#456.
@MrSerth MrSerth linked a pull request Sep 19, 2023 that will close this issue
MrSerth pushed a commit that referenced this issue Sep 19, 2023
that included a wrong usage of the `required` attribute.

See #1914 and openHPI/poseidon#456.
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.

2 participants