regression in block return types

[benoist]

In version 0.13.0 this used to work:

https://carc.in/#/r/uqv

In version 0.14.1 this throws a segfault

https://carc.in/#/r/uqx

Invalid memory access (signal 11) at address 0x579af401
[4431631259] *CallStack::print_backtrace:Int32 +107
[4431622199] __crystal_sigfault_handler +55
[140735475180842] _sigtramp +26

[asterite]

Good catch, and thank you for taking the time to reduce the issue to those few lines.

This bug alone might be a good reason to release 0.14.2 soon, because it's basically generating wrong code.

[benoist]

Thank you for fixing it so quickly :) A new release soon would be good in this case, perhaps wait another 24 hours or so to see if more regressions are found?
I'm updating my shards to see if any more pop up, this one came from carbon.

[asterite]

@benoist Good idea, we'll probably release 0.14.2 tomorrow, I found another regression regarding openssl

[sdogruyol]

Wow. I just run this issue in Kemal while handling WebSocket procs like below.

def ws(path, &block : HTTP::WebSocket -> _)
  Kemal::WebSocketHandler.new path, &block
end

This works perfectly fine in 0.13.0 but throws the following error in 0.14.1.

in ./src/kemal/websocket_handler.cr:4: instance variable '@proc' of HTTP::WebSocketHandler must be (HTTP::WebSocket, HTTP::Server::Context -> Void), not ((HTTP::WebSocket -> Void) | (HTTP::WebSocket, HTTP::Server::Context -> Void))

  def initialize(@path, &@proc : HTTP::WebSocket ->)
                         ^

================================================================================

(HTTP::WebSocket -> Void) trace:

  ./src/kemal/websocket_handler.cr:4

      def initialize(@path, &@proc : HTTP::WebSocket ->)

[benoist]

Did you try using the master branch of crystal? The regression is not fixed in 0.14.1 but only in master.

[sdogruyol]

@benoist i'm waiting for the 0.14.2 release

[benoist]

I just ran the kemal spec against master and your error message is still there.

@asterite I have a similar issue in carbon with the callbacks

[benoist]

I checked which commit causes the problems for kemal

7a32c39...b1ed447

The problem in carbon is caused by

ba6248d...a987b1b

[asterite]

@benoist @sdogruyol It's caused by this accepeted PR: #2313

Basically, now the websocket handler has also access to the HTTP::Context. We believed this was a backwards-compatible change because if you pass a block with just one block argument then it works. However, if you defined a method with the same block type and you forwarded it, you need to update it.

So, you need to change your code to this:

def ws(path, &block : HTTP::WebSocket, HTTP::Context -> _)
  Kemal::WebSocketHandler.new path, &block
end

That should solve the problem.

[benoist]

@asterite is that syntax correct?

&block : HTTP::WebSocket, HTTP::Context -> _)

What would the, HTTP::Context -> _ do?

[waterlink]

I think multiple argument types need to be in parentheses. And it is less confusing this way. BTW I didn't know you can write _ instead of Void..

Am 23.03.2016 um 12:05 PM schrieb benoist [email protected]:

@asterite is that syntax correct?

&block : HTTP::WebSocket, HTTP::Context -> _)
What would the, HTTP::Context -> _ do?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub

[benoist]

Ah right it's the arguments to the block. Yeah parens would help :-)

[asterite]

Oh, just copied and pasted and added HTTP::Context, didn't notice the _. It should actually be without the _, like this:

def ws(path, &block : HTTP::WebSocket, HTTP::Context -> ) # or Void
  Kemal::WebSocketHandler.new path, &block
end

The _ will make the method be instantiated once for every block return type, but here we want Void as a return type (ommiting the return type is the same).

https://play.crystal-lang.org/#/r/uxx

However, the compiler can implicitly convert a block that returns any type to one that returns void... because the later won't use the return value, so that's why using _ is probably working (though I'd advice to change it to Void or just nothing to avoid making those blocks return something for no use).

I should add this to the docs... 😕