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

Invalid response when body is empty #55

Closed
Dennor opened this issue Dec 6, 2017 · 8 comments
Closed

Invalid response when body is empty #55

Dennor opened this issue Dec 6, 2017 · 8 comments

Comments

@Dennor
Copy link
Contributor

Dennor commented Dec 6, 2017

Expected behaviour

HTTP/1.1 200 OK
Server: nginx
Content-Type: text/plain; charset=utf-8
Content-Length: 122
Connection: keep-alive
Keep-Alive: timeout=20
Date: Wed, 06 Dec 2017 13:13:17 GMT

When response is returned and is empty, it should stay empty.

Actual behaviour

HTTP/1.1 200 OK
Server: nginx
Content-Type: text/plain; charset=utf-8
Content-Length: 122
Connection: keep-alive
Keep-Alive: timeout=20
Date: Wed, 06 Dec 2017 13:13:17 GMT

{"id":27,"executed_at":"2017-12-06T13:13:17.344436Z","status":"success","duration":810,"result":{"stdout":"","stderr":""}}

Steps to reproduce

socket.yml

name: fail-case
description: Description of fail-case
version: 0.0.1

endpoints:
  fail-case:
    file: fail-case.js
    description: Empty response
    parameters:
    response:

src/fail-case.js

import Server from 'syncano-server';

export default async ctx => {
  const {response} = Server(ctx);
  return response('', 200, 'text/plain', {});
};

package.json

{
  "license": "MIT",
  "dependencies": {
    "syncano-server": "beta"
  },
  "devDependencies": {
    "eslint": "^4.12.1"
  }
}

Details

npx s version

0.53.0

When script returns an empty response, it replaces it with script execution info. Sometimes empty response is an expected behaviour. It can be useful for application/octet-stream and text/plain content-type responses, but it also fails at empty application/json.

@23doors
Copy link
Member

23doors commented Dec 6, 2017

Hey @Dennor! It seems that here the issue is not with the response being empty but with returning it from a Promise. This should work without async keyword. If you need async here - we should be able to add support for exporting async functions in a few days.

@Dennor
Copy link
Contributor Author

Dennor commented Dec 6, 2017

import Server from 'syncano-server';

export default async ctx => {
  const {response} = Server(ctx);
  return response('text', 200, 'text/plain', {});
};

With code like this, the response I'm getting is:

HTTP/1.1 200 OK
Server: nginx
Content-Type: text/plain; charset=utf-8
Content-Length: 4
Connection: keep-alive
Keep-Alive: timeout=20
Date: Wed, 06 Dec 2017 14:05:25 GMT

text

I'm using syncano-cli@beta by the way, so async gets transpiled away.

@23doors
Copy link
Member

23doors commented Dec 6, 2017

As I mentioned in above comment - async here is the issue. Transpiled or not - async in export is not fully handled in terms of return response.

@Dennor
Copy link
Contributor Author

Dennor commented Dec 6, 2017

import Server from 'syncano-server';

export default ctx => {
  const {response} = Server(ctx);
  return response('', 200, 'text/plain', {});
};

Still getting:

HTTP/1.1 200 OK
Server: nginx
Content-Type: text/plain; charset=utf-8
Content-Length: 120
Connection: keep-alive
Keep-Alive: timeout=20
Date: Wed, 06 Dec 2017 14:10:45 GMT

{"id":51,"executed_at":"2017-12-06T14:10:45.141419Z","status":"success","duration":4,"result":{"stdout":"","stderr":""}}

As I've said, every other response, except for response with empty body, as in:

return response('', 200, 'application/json', {});
return response('', 200, 'application/octet-stream', {});
return response('', 200, 'text/plain', {});

is returning, what it's supposed to be returning. When the body is empty I'm getting execution context state. Tested just now both, with and without async keyword.

@23doors
Copy link
Member

23doors commented Dec 6, 2017

You are right. Turns out that both handling async in backend part and incorrectly checking content are an issue here.

@Idered
Copy link
Member

Idered commented Dec 20, 2017

@Dennor @23doors I've removed content check as suggested in Syncano/syncano-server-js#99 . It will now always call underneath setResponse. I guess that resolves this issue. Right @Dennor ?

@mkucharz
Copy link
Member

@Dennor @23doors are we good here?

@mkucharz
Copy link
Member

@Dennor @23doors @Idered closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants